| [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2988
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: | Warning fixes |
Version: | 1.3-current |
Created By: | skunk |
Assigned To: | AlbrechtS |
Fix Version: | 1.3.4 (SVN: v11243) |
Update Notification: | |
Trouble Report Files:
|
#1 | skunk 21:55 Sep 28, 2013 |
| fltk-cpp-fixes.patch 15k | |
|
#2 | skunk 21:56 Sep 28, 2013 |
| fltk-const-fixes.patch 4k | |
|
#3 | skunk 21:56 Sep 28, 2013 |
| fltk-static-fixes.patch 4k | |
|
#4 | skunk 21:57 Sep 28, 2013 |
| fltk-sign-fixes.patch 4k | |
|
#5 | skunk 21:57 Sep 28, 2013 |
| fltk-misc-fixes.patch 4k | |
|
#6 | skunk 18:47 Feb 22, 2016 |
| fltk-misc-1.patch 17k | |
|
#7 | skunk 17:21 Feb 23, 2016 |
| warning-summary.pl 3k | |
|
#8 | skunk 07:29 Feb 24, 2016 |
| warning-summary.pl.txt 3k | |
|
#9 | AlbrechtS 07:34 Feb 27, 2016 |
| fl_set_font_check_fnum.diff 0k | |
|
#10 | skunk 01:17 Feb 28, 2016 |
| fltk-configure-fix.patch 10k | |
Trouble Report Comments:
|
#1 | skunk 21:55 Sep 28, 2013 |
| These patches are an outgrowth of my work on STR #2973. (Greg, by the way, thanks for getting that one in.) I noticed numerous less-harmful warnings when building FLTK on Linux/g++ with my usually aggressive flags, and wanted to get those resolved, but they were off-topic for that report and so I've created a new one for them here. All these patches are against r9994, and should be applied in the same order as they are described below:
fltk-cpp-fixes.patch: Copious warnings of the form "FOO is not defined" are produced if you compile FLTK with "g++ -Wundef". This is directly caused by undefined cpp symbols being used in #if conditionals, and in turn that is due to different categories of cpp symbols not being used in accordance with how they are defined.
Some cpp symbols, like HAVE_GL, are always defined, and either have a zero (false) or non-zero (true) value. Some other symbols, like USE_X11, are either undefined or defined (usually with a value of 1, but the value shouldn't matter). If you have a cpp conditional like "#if USE_X11", it will evaluate to false if USE_X11 is not defined, but only because the preprocessor implicitly replaces the symbol with 0. The correct way of writing that is "#ifdef USE_X11" or "#if defined(USE_X11)".
The configh.in Autoconf config header template file is, presumably, authoritative as to which category a given symbol belongs. Zero-or-not symbols are listed as e.g.
#define FOO 0
whereas defined-or-not symbols appear as
#undef BAR
(There are also some cpp symbols in FLTK that are not #defined anywhere, like HAVE_LIBC_ICONV, and these can be assumed to be in the defined-or-not category.)
My first patch, then, does the following:
* Change #if conditionals to #ifdef/#if-defined() as appropriate.
* Added logic to "#define FLTK_ABI_VERSION 0" if it is otherwise undefined, so that all the "#if FLTK_ABI_VERSION > 12345" conditionals don't give warnings. I figured this was a better way to go than to rewrite all those conditionals as e.g.
#if defined(FLTK_ABI_VERSION) && FLTK_ABI_VERSION > 12345
* Edited configh.cmake.in to be consistent with configh.in. Also, directives of the form "#cmakedefine FOO @FOO@" can be simplified to "#cmakedefine FOO" if FOO is just a true/false value.
fltk-const-fixes.patch: Several casts in FLTK were not respecting const qualifiers on their arguments.
fltk-static-fixes.patch: A handful of functions were giving "no previous declaration for `foo'" warnings (-Wmissing-declarations), which usually indicates that a function is not used outside its associated source file and thus can be declared as static.
fltk-sign-fixes.patch: A few places where a comparison or assignment is made between types of different integer-signedness without an explicit cast. This patch adds said casts.
fltk-misc-fixes.patch: A grab bag of minor tweaks, as follows:
* FL/fl_utf8.h: warning: function declaration isn't a prototype
* fluid/code.cxx: warning: format string is variable, can't check
* fluid/fluid.cxx: ditto
* src/numericsort.c: I was getting a "no previous declaration" warning on fl_casenumericsort() and fl_numericsort(), and there's no reason not to #include the header file outside of WIN32, right?
* src/scandir.c: warning: empty translation unit (I stuck the typedef on the end so not to make the cpp conditional logic noisier. It's harmless even if there's fl_scandir() source code above.) | |
|
#2 | cand 07:06 Aug 21, 2014 |
| I've applied your static-fixes patch with the exception of right2left, which was mentioned in Xutf8.h.
Your numericsort fix is something I also stumbled upon, and had produced the exact same code. It was committed earlier today, as it was necessary for a visibility build on Linux. | |
|
#3 | skunk 20:33 Aug 25, 2014 |
| Thanks for getting those in. Don't forget the other patches, too! I'd like to get the FLTK build as clean as possible. | |
|
#4 | ianmacarthur 03:51 Sep 05, 2014 |
| Lauri, is this one ready to close? | |
|
#5 | cand 04:46 Sep 05, 2014 |
| No, as 3.9 out of 5 patches remain to be looked at. I only pulled what I was working on at the time (xutf8). | |
|
#6 | AlbrechtS 19:00 Jan 30, 2016 |
| I committed another bunch of changes in svn r11094: similar, but not identical to fltk-cpp-fixes.patch.
Meanwhile I had changed some of the config variables (configh.in, configh.cmake.in) in a different way, so the resultant patch is not directly comparable, but in my compilations with -Wundef all warnings are gone now (Linux + MinGW).
Can you confirm this?
FLTK_ABI_VERSION is now always defined, so that part is obsolete.
Can you please tell me which "usually aggressive flags" you are using exactly so I can reproduce the rest of the warnings. This would help very much to check and verify your remaning patch(es). TIA. | |
|
#7 | AlbrechtS 19:36 Jan 30, 2016 |
| I tried to enable many warnings with "-Wall -W", but then I got a few warnings from image libs under Windows (e.g. -Wclobbered) and lots of missing-field-initializers and unused-parameter warnings. There are also one or two deprecated-declarations warnings which are harmless, but we can't fix them. I don't care about missing-field-initializers and unused-parameter warnings. Unused parameters are harmless, but I didn't check any of the missing-field-initializers warnings. Maybe this is all intentional?
So I compiled with:
-Wall -W -Wno-missing-field-initializers -Wno-deprecated-declarations -Wno-clobbered -Wno-unused-parameter
... under Linux with absolutely no warnings and
... under Windows (MinGW) with three (3) warnings (-Wsign-compare) in src/vsnprintf.c, but these are trivial to fix (comparison with (unsigned) sizeof()).
If you don't have any compelling reasons to keep this STR open, then I'd like and suggest to close it. If there are reasons, please post your compiler flags, but I can't tell if any changes will make it into FLTK 1.3.4. Let's see... | |
|
#8 | AlbrechtS 19:42 Jan 30, 2016 |
| [-Wsign-compare] warnings fixed in svn r 11095. | |
|
#9 | skunk 19:02 Feb 22, 2016 |
| Hi AlbrechtS,
There are a few remaining issues that I've addressed in my tree. Thanks for looking at the previous ones.
First, there's something I need to ask: Currently, some of the config-header symbols are undef-or-1, and some are 0-or-1. Could we make them all undef-or-1? The inconsistency is difficult to work with, and Autoconf has long standardized on undef-or-1. It's particularly bad in that even the HAVE_XXX_H ("do we have header xxx?") symbols are split between the two conventions. I'd like to fix this.
For now, I've posted a patch (fltk-misc-1) with a grab bag of things to look at:
* Add "void" to a function decl to avoid "function declaration isn't a prototype" warning
* Several instances of for-loops with a semicolon instead of a body; should be replaced for clarity with "{ //nothing }" like you've already done in a few places
* fl_rect.cxx: It was long ago, but at one point I ran through this code with a NULL fl_gc, and the application crashed. This might be a prudent thing to add.
* fl_set_font.cxx: Really should check the range of fnum here.
* fl_utf.c: Fix several const snafus, and some char assignments. Also, this file is C, so no C++ comments.
* scandir.c: Some compilers don't like an empty source file.
* screen_xywh.cxx: Those vars should be initialized if we're going to pass them to a function.
* vsnprintf.c: C file == no C++ comments.
* src/xutf8/case.c, src/xutf8/is_spacing.c: Need to #include Xutf8.h or else we get "no previous declaration" warnings (-Wmissing-declarations)
* src/xutf8/utf8Wrap.c: More const lossage. | |
|
#10 | AlbrechtS 03:58 Feb 23, 2016 |
| Hi Daniel,
thanks for the update. Your patch looks plausible, but I didn't check it thoroughly yet. I'll try to get this in FLTK 1.3.4 (supposedly the final 1.3.x release).
As I wrote before I'm still interested in your (hopefully gcc) compiler flags to find all these warnings - or are you using a set of compilers (maybe including very old ones)?
PS: taking ownership of this STR. Lauri, I hope you don't mind... | |
|
#11 | skunk 13:16 Feb 23, 2016 |
| I can't say for sure what flags I was using before (my bookkeeping has suffered due to various relocations in the past couple years), but here is what I've been using for now. They're for GCC:
CFLAGS='-pedantic -pipe -fno-common -W -Wall -Wcast-align -Wformat=2 -Wpointer-arith -Wundef -Waggregate-return -Wcast-qual -Wmissing-declarations -Wnested-externs -Wstrict-prototypes -O3'
CXXFLAGS='-pipe -fmessage-length=0 -fno-rtti -W -Wall -Wcast-align -Wformat=2 -Wpointer-arith -Wundef -O3'
(I do have older compilers on arcane Unix systems that I've built on in the past; would you like me to screen for issues there again?) | |
|
#12 | AlbrechtS 16:13 Feb 23, 2016 |
| Thanks for the compiler flags. That's an overwhelming set. I tried compiling with these flags w/o your patch and with the patch, and I can confirm that the patch fixes all warnings. I'd like to check docs of some of the flags later though...
Looking at the patch details: I believe that everything concerning compiler warnings is okay, but it's too late now (01:15 am) to get to a final result. I'll check this more thorouhgly later and likely commit the entire patch except the part mentioned in the next post (separating posts deliberately for clarity). | |
|
#13 | AlbrechtS 16:16 Feb 23, 2016 |
| The following patch is "incomplete" and needs investigation:
> * fl_set_font.cxx: Really should check the range of fnum here.
Reasoning:
(1) this is not a compiler warning issue (2) this patch is incomplete - it should likely have an own STR
The problem (AFAICT now) is: checking <fnum> is okay, but this is not enough.
Imagine you call Fl::set_font(Fl_Font fnum, Fl_Font from) - a few lines above Fl::get_font(Fl_Font fnum) - with an out-of-range <from> value. Fl::get_font(..) returns 0, and Fl::set_font(Fl_Font fnum, const char* name) will be called with a NULL <name> argument. This will result in calling strcmp(s->name, name) (at line #61) with name == NULL.
We could check "name != NULL", but I have no idea what Fl::set_font(Fl_Font fnum, const char* name) should do if name == NULL. Initialize the font struct with some defined "null" values, crash, ... ?
An alternative would be to _use_ your patch, but file an additional STR for the remaining issues, or ... fix the remaining issues as well. | |
|
#14 | AlbrechtS 16:26 Feb 23, 2016 |
| Regarding comment #11 (arcane Unix systems): No, I don't want you to check these systems (now). I'd like to fix as many issues as possible and close this STR before we release FLTK 1.3.4. No more issues in this STR, please!
FYI: we know that we can't easily merge future changes in branch-1.3 into branch-1.3-porting (the future FLTK 1.4.0). However, we don't want to lose any fixes done in 1.3.x, so we must fix this in branch-1.3-porting as well. Hint: if you like, then you may "port" your patch to branch-1.3-porting if it doesn't apply cleanly (I didn't test it yet) and post the modified patch here to simplify things ... for me ;-) (TIA) | |
|
#15 | skunk 17:21 Feb 23, 2016 |
| I've attached a script (warning-summary.pl) which I regularly use to wrangle warnings. Run it on your build log and it'll give you a concise overview of what's in there.
The patch isn't meant so much to be applied as-is, but with some manual finishing. For example, you won't want to just convert ";" into "{}" after the loop specifiers, but expand "{}" onto separate lines with a comment in between (like you've done elsewhere), and so on.
The fl_set_font.cxx thing is just a related issue I caught along the way (using a liberal definition of "warning" here... a good static analyzer would probably warn about this :) I don't pretend to know the best way to fix this, nor that this fixes everything, but obviously there's a couple places here that could use some hardening. I can file a new STR if you think this deserves one.
I can help with porting the fix to 1.3-porting, though that should be the as-committed changes instead of what's in my patch. If you let me know which commit(s) in 1.3 are relevant, I'll be happy to do that. | |
|
#16 | AlbrechtS 03:27 Feb 24, 2016 |
| Raised priority from low (which is correct) to high temporarily to be sure we get this into FLTK 1.3.4.
@Daniel: I'll come back to your comments later. Can you please upload warning-summary.pl again with a different name (e.g. warning-summary.pl.txt) - we get an internal server error when accessing it, and I suspect that the ".pl" file extension causes this. TIA. | |
|
#17 | skunk 07:30 Feb 24, 2016 |
| .pl.txt file posted. Do check that the server wasn't trying to run my script as a cgi-bin or something... that would be a pretty severe security hole :> | |
|
#18 | AlbrechtS 11:06 Feb 24, 2016 |
| Hole closed. | |
|
#19 | AlbrechtS 07:21 Feb 27, 2016 |
| Fixed in Subversion repository.
I committed all changes in fltk-misc-1.patch (svn r11243) with these modifications:
- src/fl_set_fonts.cxx: patch ignored - {} replaced with {/*empty*/} as suggested - one additional fix in src/Fl_Image_Surface.cxx
This is the final commit for this STR, unless there are compelling reasons to change even more (maybe regressions).
Please report if this is okay for you. | |
|
#20 | AlbrechtS 07:34 Feb 27, 2016 |
| Todo:
(1) Patch of fl_set_fonts.cxx: see attached file fl_set_font_check_fnum.diff.
Please feel free to open another STR for this one.
(2) You wrote in comment #9: "Currently, some of the config-header symbols are undef-or-1, and some are 0-or-1. Could we make them all undef-or-1?"
I agree that there are some inconsistencies, and it would be easier to write (correct) code if we had consistent "undef-or-1" or "0-or-1" definitions. When I changed the code and the definitions recently I tried to fix the code _or_ the definitions, whatever seemed to be simpler or shorter code, or the smaller number of changes.
Since this is now correct AFAICT this is not going to be touched again in FLTK 1.3.x - however I'd like to check this again in FLTK 1.4. Please feel free to file a new STR for FLTK 1.4 (1.4-feature) so we don't forget this.
Thanks for your contributions again and, BTW, I'm using your compiler flags and occasionally warning-summary.pl. It's very helpful.
Last, but not least: I'm going to install the patch in branch-1.3-porting as well. A quick test showed that it can be applied almost as it is. | |
|
#21 | AlbrechtS 09:34 Feb 27, 2016 |
| FTR: All warning fixes are now also in branch-1.3-porting (committed in svn r11246). | |
|
#22 | skunk 01:18 Feb 28, 2016 |
| Okay, that looks great. I'll open new STRs for the fl_set_fonts.cxx bit, and the undef/0-vs-1 issue in 1.4.
Glad to hear the flags and script are useful :) Keeping tabs on warnings really helps with QA, especially since a warning from GCC/Linux often becomes an error in a different build environment. You might consider putting together a standard set of (development-only) build flags for FLTK, with something like
-Werror -Wno-error=unused-parameter -Wno-error=missing-field-initializers ...
(i.e. throw an error for all warnings except for this limited set). That'll make building FLTK like building Java code, where issues are immediately flagged and you _have_ to fix them. I do this at my day job, to help keep in line a bunch of developers who don't necessarily care about warnings/bugs as much as I do :]
One more thing I'd like to address in 1.3: the Autoconf script has numerous issues. For one, it should be named configure.ac, because configure.in is the old name and automake now complains about this. Secondly, if you modify the autoconf invocation in autogen.sh by adding "--warnings=all", you get several warnings about obsolete macros and dodgy syntax.
I've attached a patch that cleans all that up. Think that could go in? I'll open a new STR for it if you like. | |
|
#23 | AlbrechtS 14:09 Feb 28, 2016 |
| Thanks for the -Wno-error=... hint. I knew -Werror, but not -Wno-error=... Learning something new every day.
Please open another STR for the configure stuff. I'm a little wary of touching the autoconf/configure stuff, because I know so little about it. It's not my intention to learn more about it though, since I want to concentrate on CMake (which I'm also using in commercial projects).
So all contributions to the autoconf system are appreciated, but I'm not sure if we should change the current system in FLTK 1.3. Besides potential bugs this is also a question of compatibility with very old systems (sometimes used for embedded systems or whatever).
This STR is now closed w/resolution. Further comments regarding autoconf should go to the new (to be created) STR.
TIA. Albrecht | |
[ Return to Bugs & Features ]
|
| |