stricter compiler warnings

[Kurt Schwehr]

I've been investigating turning on more warnings when compiling with clang on mac osx 10.9. I'll be using this as the master ticket to track overall process on working through the things found by this process. Most of the items are not significant with the majority being unexciting unused arguments. There are a few that might be actual bugs.

CPPFLAGS='-Wall -Wextra' ./configure ...

And then adding -Werror to GDALmake.opt after it's built. Adding -Werror breaks the configure process with vsnprintf.

grep ignored gdal-strict-2014-03-09.patch | wc -l
507

grep pragma gdal-strict-2014-03-09.patch  | grep ignored | sort -u
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#pragma GCC diagnostic ignored "-Wdeprecated-writable-strings"
+#pragma GCC diagnostic ignored "-Wempty-translation-unit"
+#pragma GCC diagnostic ignored "-Wenum-conversion"
+#pragma GCC diagnostic ignored "-Wextra-semi"
+#pragma GCC diagnostic ignored "-Wgnu"
+#pragma GCC diagnostic ignored "-Winvalid-source-encoding"
+#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
+#pragma GCC diagnostic ignored "-Woverloaded-virtual"
+#pragma GCC diagnostic ignored "-Wpedantic"
+#pragma GCC diagnostic ignored "-Wsign-compare"
+#pragma GCC diagnostic ignored "-Wswitch"
+#pragma GCC diagnostic ignored "-Wtautological-compare"
+#pragma GCC diagnostic ignored "-Wtautological-constant-out-of-range-compare"
+#pragma GCC diagnostic ignored "-Wunneeded-internal-declaration"
+#pragma GCC diagnostic ignored "-Wunused-parameter"

Sample diff:

 /*                                Stat()                                */
 /************************************************************************/
 
+#pragma GCC diagnostic ignored "-Wunused-parameter"
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 int VSIUnixStdioFilesystemHandler::Stat( const char * pszFilename, 
                                          VSIStatBufL * pStatBuf,
                                          int nFlags)
@@ -508,6 +512,8 @@
 {
     return( VSI_STAT64( pszFilename, pStatBuf ) );
 }
+#pragma GCC diagnostic error "-Wunused-parameter"
+#pragma GCC diagnostic error "-Wdeprecated-declarations"

[Kurt Schwehr]

2/3rds of the way through compiling with stricter flags

[Even Rouault]

Hum, I'm sorry you have spent all this energy to do that, but I find the end result quite bad looking. Do we really want to pollute GDAL sources with all those pragma ? Why not just telling the compiler -Wno-unused-parameter... ? Well, I don't pretend to have the last say on this, so you should raise this on gdal-dev.

[Kurt Schwehr]

Hey Even, That's what I get for dashing out a quick message. I definitely DO NOT drop that ugly junk onto gdal. I agree that is it super ugly and will also break the build with some compilers. Hopefully what follows makes more sense. There are multiple ways for some of these issues, so I look forward to discussions.

For things like tautologies, a quick fix to the code is all that is needed. e.g. (note... upgrading degrib would also remove this)

--- frmts/grib/degrib18/degrib/myutil.c (revision 27020)
+++ frmts/grib/degrib18/degrib/myutil.c (working copy)
@@ -666,7 +668,9 @@
 
    /* Remove the trailing white space before working on the leading ones. */
    len = strlen (str);
-   for (i = len - 1; ((i >= 0) && (isspace ((unsigned char)str[i]))); i--) {
+   for (i = len - 1; ((i > 0) && (isspace ((unsigned char)str[i]))); i--) {
    }
    len = i + 1;
    str[len] = '\0';
@@ -715,7 +719,9 @@

A quick not fully thought out example of what I would like to propose (and totally open to counter proposals) for the unused-arguments:

Create a macro UNUSED or GDAL_UNUSED that is set based on which compiler. Here I'm only showing gcc / clang:

#ifdefined(__GNUC__) && __GNUC__ >= 4
#define UNUSED __attribute((__unused__))
#else
/* TODO: add cases for other compilers */
#define UNUSED
#endif

Then the first example I had becomes:

int VSIUnixStdioFilesystemHandler::Stat( const char * pszFilename, 
                                          VSIStatBufL * pStatBuf,
                                          UNUSED int nFlags)

I would say that most of the unused cases, it seems obviously fine that the argument isn't needed, but for cases where it's not clear, I plan to see if I can figure it out and if not I'll ask what's going on with the code.

I see that this type of thing is already being done... e.g. cpl_port.h

#if defined(__GNUC__) && __GNUC__ >= 4
#  define CPL_CVSID(string)     static char cpl_cvsid[] __attribute__((used)) = string;
#else
#  define CPL_CVSID(string)     static char cpl_cvsid[] = string; \
static char *cvsid_aw() { return( cvsid_aw() ? ((char *) NULL) : cpl_cvsid ); }

[Even Rouault]

The upgrade of degrib would be much needed, but not trivial. We have so many patches in it that you cannot just drop-in the new version. I had tried but forgave... Several days of work involved.

Yes your proposal of UNUSED macro would be better. CPL_UNUSED to be consistant with other similar macros ? Not sure if MSVC has similar mechanism but the GCC one would be already cool. I've seen some code in other projects that write (void)the_unused_variable; as statements at the beginning of functions, but I think modern compilers also complain about that with some warning options...

[Kurt Schwehr]

Thanks for the CPL_UNUSED suggestion. That makes a lot of sense and would fit at the end of source:trunk/gdal/port/cpl_port.h#596​

I figured that degrib updating is probably not trivial. In the short term, I will focus on the couple minor cleanups that I found in degrib18.

[Kurt Schwehr]

Added CPL_USED: r27702 and r27701 for 1.11 branch (> 1.11.0)

[Kurt Schwehr]

First round of fixes for compiler warnings: r27721

Which covers these changes to 1.11.1: r27703 r27704 r27705 r27706 r27707 r27708 r27709 r27710 r27711 r27712 r27717 r27718 r27720

r27716 was already fixed in trunk.

I'm using:

gcc --version
Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)

xcodebuild -version
Xcode 5.1.1

[Kurt Schwehr]

Working primarily on 1.11-svn since this is about not changing functionality. Changes to trunk will be a bit more aggressive.

After doing a whole bunch of small fixes to verify that things basically work, I've done larger changes to the 1.11-svn tree: r27722, r27723 and r27729. I backed out part of the change to ogrsqliteexecutesql.cpp in r27730. I was working without spatialite, so I goofed that.

I've had to skip a lot of warning for now because the fixes were either more involved or risked possible. -Wno-invalid-source-encoding -Wno-deprecated-writable-strings -Wno-switch -Wno-unused-function -Wno-overloaded-virtual -Wno-missing-field-initializers -Wno-format -Wno-enum-conversion -Wno-missing-field-initializers -Wno-sometimes-uninitialized

[Even Rouault]

Kurt, I had to commit : branches/1.11 "Fix compilation errors due to C99'isms (#5414)"

I appreciate your efforts on cleaning warnings, but this makes me feel really nervous to have such breakage the day where I had advertized my intent to cut a 1.11.1 RC1... Per http://trac.osgeo.org/gdal/wiki/rfc3_commiters "Normally only fixes should go into stable branches". Perhaps you could maintain a dedicated 1.11 branch to experiment with the warning cleansing ?

I'd also want to mention that you've made changes to third-party code imported in GDAL, such as libgeotiff. The changes should go in the upstream project too, otherwise they will be lost the next time we resync the code with upstream libgeotiff.

[Kurt Schwehr]

Even, Sorry for any hassles I've create and especially for the c99'isms that you fixed in r27731.

I was trying to get these changes in before the 1.11.1. If I can get the compiler warnings under control, then I can start working on deeper analysis of GDAL and maybe get a chance to work on the Coverity list. But I can't do that on trunk. I only get one version in the environment where I can best do the analysis. Everything in the changes I have submitted has meant to be just handling compiler warnings. There should be zero behavior changes. I won't make any more changes to 1.11 for the near future (the last one was r27741). There are 10's of warnings left, but they are more difficult to fix. The remaining warnings and anything I find doing analysis on 1.11.1 will be have a bug created, be fixed first in trunk and then be back ported to 1.11.1 only after discussion.

I definitely intend on getting any changes made to the third party libraries tracked. In fact, one change I found for libtiff was already in upstream, but somehow did not make it to gdal. I will work with the libraries to find solutions that work for them. If there is no upstream solution, I may get to continually forward port these changes on each if I want to keep those libraries building warning free. e.g. I have a whole stack of proposed changes for hdf4 and hdf5 and little hope of getting those changes (or the HDF teams' version of those changes into the libraries). e.g. ​https://github.com/schwehr/hdf4/commits/master

r27745 finishes for trunk doing all of what I did for 1.11

[Even Rouault]

trunk r27794 "Extra fixes for unused parameters (#5414)"

[Kurt Schwehr]

I started cleaning degrib 2.03 from the latest release (no repo online) and have passed a patch upstream to NOAA. ​https://github.com/schwehr/degrib/commits/clean_mac

[Even Rouault]

Any reason keeping that ticket open ? Extra (non-controversial) warning cleanup can go to trunk without ticket. The degrib one can deserve its own ticket