| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2714
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 2 - Low, e.g. a documentation error or undocumented side-effect |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | FLTK headers have shadow lint |
|---|
| Version: | 1.3-feature |
|---|
| Created By: | ldoolitt |
|---|
| Assigned To: | matt |
|---|
| Fix Version: | 1.4.0 (SVN: v9102) |
|---|
| Fix Commit: | c97990e517adca2d0a8c287bddbdd4761f5965f2 |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | ldoolitt
08:50 Sep 19, 2011 |
| Building application code with FLTK can't reasonably use gcc's -Wshadow, because FLTK's headers have shadow lint. While it would be nice if the entire FLTK code base were warning-free even with -Wshadow, the attached patch just fixes the headers, so application code can turn on that warning switch without chatter from the FLTK headers. Patch against fltk-1.3 svn 9049. | |
| |
| #2 | greg.ercolano
14:58 Sep 22, 2011 |
| Not taking ownership of this STR, but I like the goal, prevent variable shadowing in application code; that's a good point.
Not crazy about all those underbars though.
Sounds like a good reason to get code out of the .H's.
I can say for the case of Fl_Tree.H, much of the multiline code
that's there is leftover from dev; when I start developing
a class, I often start with all the code just in the .H
while I flesh it out, then later move big method code to .cxx,
but leave the smallish stuff in .H, partly from laziness, and
partly for maintenance.
Anyway, I'll fix the Fl_Tree.H code with shadows in mind.
Will see what others have to say about the other parts of the patch. | |
| |
| #3 | matt
15:25 Sep 22, 2011 |
| I agree that it would be nice to have now shadowing, but I also don't like the underscores. I have seen naming schemes that prepend or append an A for argument, using various kinds of Camel Humps.
draw(int aX int aY, int aWidth, int aHeight)
The issue is with Doxygen though. A method like draw(int x, int y, int w, int h) is kind of self explanatory, and using meaningful and readable variable names is very helpful here.
The other options would be to use quite verbose names, but who will do all the typing, for example:
draw(int xPosition, int yPosition, int width, int height)
and even with those we may get shadowing... . | |
| |
| #4 | ldoolitt
15:27 Sep 22, 2011 |
| Hey, I'm not crazy about all those underbars either. If someone has a better style to suggest, I'm receptive. My goals were a self-contained patch, and not to introduce a bunch of random new names that could also be prone to conflict. Please note that whatever new name is used for this purpose in these headers can not generate shadow warnings (or any other warning) in the user code itself; any warnings come from user or other header declarations leaking into the compilation of the FLTK header itself. | |
| |
| #5 | greg.ercolano
16:12 Sep 22, 2011 |
| @matt: perhaps the problem can be avoided in many cases
by simply moving the method code out of .H and into the .cxx.
But hrm, does moving implemented code from .H to .cxx
affect the ABI? Even though the interfaces aren't changing,
I'm not sure if code implemented in .H doesn't end up in the
.o/.objs, in which case it may affect runtime linkage?
I'm going to hold on committing my changes to Fl_Tree
which moves just about all the code from .H -> .cxx until
I hear back on this, as I'm not sure.. | |
| |
| #6 | greg.ercolano
12:12 Sep 29, 2011 |
| According to the thread on fltk.development, it sounds like moving code from .H -> .cxx won't affect the ABI, and may improve code maintainability (regarding the affect on DLLs for consistency in code use)
So when I get a chance, I'm going to commit the mods to Fl_Tree, which is such a new widget, that a large code re-org shouldn't be an issue. This should solve the variable shadowing for app code, while not needing to change the variable names. | |
| |
| #7 | matt
09:30 Oct 01, 2011 |
| After a test compile with -Wshadow I'll have to say: this will be painful to fix with sensible names! | |
| |
| #8 | matt
09:42 Oct 01, 2011 |
| Upped to 1.4 because this will not make it into 1.3.1 or 1.3.2 . It would be nice though to be able to have as many compiler warnings enabled without getting any warnings... . | |
| |
| #9 | greg.ercolano
11:22 Oct 01, 2011 |
| Matt: wait, are you sure it needs to be 1.4?
I think this str can be solved within the 1.3.x series
without variable name changes by simply moving source in the .H
for the methods he's cited from .H -> .cxx.
Note he's not trying to get the fltk lib to compile with -Wshadow,
just his own apps.
So that could just mean relocating code, without changing anything.
(I've already modified Fl_Tree's in r9085, and working on
Fl_Table. In my case I moved all the code, not just his methods,
but for other fltk files, small amounts of code could be moved)
I don't think we need to bump to 1.4 to fix this; no abi issues.
It can probably be held for 1.3.2, unless the plan is to go
for a 1.4.x asap. I figure there's still room for a few maintenance
releases for 1.3.
To test for shadow problems for apps, we could make one test program
that #include's all fltk widgets, instancing them once, and have just
that test program compile with -Wshadow. This way future builds
will note shadow errors in our .H's. | |
| |
| #10 | matt
11:47 Oct 01, 2011 |
| Oh, I see. I misunderstood. Back to 1.3.2 . | |
| |
| #11 | matt
23:28 Oct 01, 2011 |
| Fixed in Subversion repository.
Please let me know if I caught them all. | |
| |
| #12 | greg.ercolano
09:15 Oct 04, 2011 |
| I've made a new STR #2728 which covers my suggestion above: make a test program that #include's all the fltk widgets, compiling with -Wshadow so we have a way to always check for such errors. | |
| |
| #13 | greg.ercolano
09:29 Oct 04, 2011 |
| Just checked out the latest svn, made a program called 'shadow_variables.cxx' (attached here) in the test directory that simply #include's all the files in the FL directory (except mac.H, win32.H and x.H), and built it with -Wshadow.
Got these, so I guess there's still a few:
$ g++ -I.. -Wshadow -Os -Wall -Wunused -Wno-format-y2k -fno-exceptions -fno-strict-aliasing -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_THREAD_SAFE -D_REENTRANT -I/usr/include/freetype2 -I/usr/include/freetype2 -c shadow_variables.cxx -o shadow_variables.o
../FL/Fl_Fill_Dial.H: In constructor 'Fl_Fill_Dial::Fl_Fill_Dial(int, int, int, int, const char*)':
../FL/Fl_Fill_Dial.H:32: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Fill_Dial.H:32: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Fill_Dial.H:32: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Fill_Dial.H:32: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Fill_Slider.H: In constructor 'Fl_Fill_Slider::Fl_Fill_Slider(int, int, int, int, const char*)':
../FL/Fl_Fill_Slider.H:31: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Fill_Slider.H:31: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Fill_Slider.H:31: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Fill_Slider.H:31: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Hor_Fill_Slider.H: In constructor 'Fl_Hor_Fill_Slider::Fl_Hor_Fill_Slider(int, int, int, int, const char*)':
../FL/Fl_Hor_Fill_Slider.H:30: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Hor_Fill_Slider.H:30: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Hor_Fill_Slider.H:30: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Hor_Fill_Slider.H:30: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Hor_Nice_Slider.H: In constructor 'Fl_Hor_Nice_Slider::Fl_Hor_Nice_Slider(int, int, int, int, const char*)':
../FL/Fl_Hor_Nice_Slider.H:30: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Hor_Nice_Slider.H:30: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Hor_Nice_Slider.H:30: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Hor_Nice_Slider.H:30: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Line_Dial.H: In constructor 'Fl_Line_Dial::Fl_Line_Dial(int, int, int, int, const char*)':
../FL/Fl_Line_Dial.H:30: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Line_Dial.H:30: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Line_Dial.H:30: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Line_Dial.H:30: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Nice_Slider.H: In constructor 'Fl_Nice_Slider::Fl_Nice_Slider(int, int, int, int, const char*)':
../FL/Fl_Nice_Slider.H:30: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Nice_Slider.H:30: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Nice_Slider.H:30: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Nice_Slider.H:30: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Radio_Round_Button.H: In constructor 'Fl_Radio_Round_Button::Fl_Radio_Round_Button(int, int, int, int, const char*)':
../FL/Fl_Radio_Round_Button.H:30: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Radio_Round_Button.H:30: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Radio_Round_Button.H:30: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Radio_Round_Button.H:30: warning: declaration of 'x' shadows a member of 'this'
../FL/Fl_Simple_Counter.H: In constructor 'Fl_Simple_Counter::Fl_Simple_Counter(int, int, int, int, const char*)':
../FL/Fl_Simple_Counter.H:34: warning: declaration of 'h' shadows a member of 'this'
../FL/Fl_Simple_Counter.H:34: warning: declaration of 'w' shadows a member of 'this'
../FL/Fl_Simple_Counter.H:34: warning: declaration of 'y' shadows a member of 'this'
../FL/Fl_Simple_Counter.H:34: warning: declaration of 'x' shadows a member of 'this'
$ | |
| |
| #14 | greg.ercolano
09:30 Oct 04, 2011 |
| Reopening. (sorry matt) | |
| |
| #15 | greg.ercolano
09:36 Oct 04, 2011 |
| I'll see if I can fix the ones I just reported.
Matt: note that changing variable names in prototypes affects the docs, which often cite the variables by name. So for instance:
/**
Sets the default text size (in pixels) for the lines in the browser to \p size.
*/
- void textsize(Fl_Fontsize size) { textsize_ = size; }
+ void textsize(Fl_Fontsize newSize) { textsize_ = newSize; }
..in that case the "\p size" would need to be changed to "\p newSize".
I think I'll just fix the code for now but not the docs, just to be consistent with your mods. We can always fix the docs in a separate commit. | |
| |
| #16 | greg.ercolano
09:46 Oct 04, 2011 |
| OK, fixed the new errors in r9127.
Surprisingly easy those; all were constructors that just needed xywhl -> XYWHL.
Leaving this open in case matt wants to update the doxygen docs for his commit. Matt, if you don't have the time, I can do it.
At some point I'll check in the test program as part of solving STR #2728. | |
| |
| #17 | greg.ercolano
13:04 May 11, 2013 |
| We still have some shadow variable errors.
Using the Makefile.patch in STR#2728, I get these errors:
$ cd /usr/local/src/fltk-1.3.x-svn/test
$ make shadow_variables
echo '#include <stdio.h>' > shadow_variables.cxx
(cd ..; perl -e 'opendir(DIR,"FL/"); foreach(sort(readdir(DIR))) { if( /\.H$/ && !/^mac.H$/ && !/^win32.H$/ && !/^x.H$/) {print "#include <FL/$_>\n"; } }' ) >> shadow_variables.cxx
echo 'int main() { return(0); }' >> shadow_variables.cxx
g++ -I.. -Wshadow -Os -Wno-deprecated-declarations -Wall -Wunused -Wno-format-y2k -fno-exceptions -fno-strict-aliasing shadow_variables.cxx -o shadow_variables ../lib/libfltk.a
In file included from shadow_variables.cxx:20:
../FL/Fl_Device.H: In member function ‘virtual void Fl_Graphics_Driver::draw(const char*, int, int, int)’:
../FL/Fl_Device.H:244: warning: declaration of ‘n’ shadows a member of 'this'
../FL/Fl_Device.H: In member function ‘virtual void Fl_Graphics_Driver::draw(const char*, int, float, float)’:
../FL/Fl_Device.H:246: warning: declaration of ‘n’ shadows a member of 'this'
../FL/Fl_Device.H: In member function ‘virtual void Fl_Graphics_Driver::draw(int, const char*, int, int, int)’:
../FL/Fl_Device.H:249: warning: declaration of ‘n’ shadows a member of 'this'
../FL/Fl_Device.H: In member function ‘virtual void Fl_Graphics_Driver::rtl_draw(const char*, int, int, int)’:
../FL/Fl_Device.H:251: warning: declaration of ‘n’ shadows a member of 'this'
../FL/Fl_Device.H: In member function ‘virtual double Fl_Graphics_Driver::width(const char*, int)’:
../FL/Fl_Device.H:385: warning: declaration of ‘n’ shadows a member of 'this'
In file included from ../FL/Fl_Double_Window.H:25,
from shadow_variables.cxx:22:
../FL/Fl_Window.H: In member function ‘void Fl_Window::size_range(int, int, int, int, int, int, int)’:
../FL/Fl_Window.H:327: warning: declaration of ‘aspect’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘dh’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘dw’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘maxh’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘maxw’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘minh’ shadows a member of 'this'
../FL/Fl_Window.H:327: warning: declaration of ‘minw’ shadows a member of 'this' | |
| |
| #18 | ianmacarthur
16:23 Mar 11, 2014 |
| Are we still pursuing these, or..?
We can keep this STR open, or close it and start again? | |
| |
| #19 | matt
08:52 Feb 02, 2019 |
| GitHub ID 452a410a3ea02f58930c4b3cc5a04bbb6b3e7070
Removed some more shadow lint. Not sure either if we should pursue this all the way. On the plus side, I found more than one place where shadowing a global variable was quite confusing. On the minus side, this introduces lots of minor changes in the code, plus all code generate by Fluid has andless lines of shadow lint, and that's impossible to change. | |
| |
| #20 | matt
13:31 Feb 02, 2019 |
| I guess I could not let this go. Anyway, all the header files are now lint free, which can't be said of my belly button. | |
[ Return to Bugs & Features ]
|
| |
