Compiling clean with -Wsign-compare

[springmeyer]

-Wsign-compare is often a good indication of subtle bugs. We should enable this warning and work to clean up all warnings that come from it (supressing those outside mapnik).

After that ideally we'd also start using all of these: -Wextra -Wconversion -Wshadow -Wunused-parameter -Wsign-compare.

[HolgerJeromin]

Or even -Wextra

[springmeyer]

noting: -Wsign-compare may not catch all bugs that could result from unexpected input or just plain unsafe comparisons across signed and unsigned numbers if static_cast has already been placed in the code to "fix" warnings. We should re-read http://jwwalker.com/pages/safe-compare.html and http://craighenderson.co.uk/papers/numeric_compare/ and try to find any places in the code where static_cast before a comparison might be misused and instead we should be using numeric_compare.

[springmeyer]

after bb6c9a9 and b2c85e0 mapnik compiles cleanly with -Wsign-compare -Wshadow.

But -Wconversion still need major work (-Wconversion is shorthand for -Wsign-conversion and -Wshorten-64-to-32)

I think the next steps here are:

• create wrapper includes around the third party includes that trigger the most warnings we want to ignore (in order to focus on our own): boost spirit, boost regex, boost interprocess, etc. This will tame the chaos of needing to disable warnings for each and every #include <boost/spirit/qi.hpp> individually and rather allow us to turn off warnings (when we need) in just one place.
• Next take aim at -Wsign-conversion which uncovers lots of places in the mapnik core code we should be cleaner and more careful about conversions. A few bugs or edge cases may pop out or be fixed in this pass.

[lightmare]

What's -Wshadow good for? I mean, seriously, although I may be biased because every time I try to compile with gcc-4.8 (which has bugged warning suppression), it spews these warnings all over agg, the silliest of which are:

warning: declaration of ‘y’ shadows a member of 'this' [-Wshadow]

That's as useless as a warning can get. this->y is still accessible. It's worse than useless, it's counter-productive. The common "fix" to silence this warning -- renaming local y to something like y_ -- makes matters worse, because then it's much easier to confuse which is which and use the wrong one.

[artemp]

@lightmare is right! It hurts my eyes to see signatures like below :)

void set(double _val)
{
    val_ = _val;
}

@springmeyer ^

[springmeyer]

What's -Wshadow good for?

This got added because I turned it on thinking it might be useful. And it immediately caught code in mapnik like:

  int i = 0;
  int j = 0;

  for (int i=0;i<10;++i) {
    int j = 4;
  }

  std::clog << "i " << i << "\n";
  std::clog << "j " << j << "\n";

Where variables were similarly named in clashing scopes. Now, I admit that running this above testcase in clang++ (at least) does not exhibit any bug. You would worry that the ++i or the int j = 4; would impact the final values of i and j where they are printed. But I see that they are - as you would hope - both 0 when printed. Which is good. But either way I fixed these out of concern that the variable values could be clobbered without the developer knowing it.

warning: declaration of ‘y’ shadows a member of 'this' [-Wshadow]

Right, the g++ warnings are awful and unhelpful. And not at all the kind of shadowing that I added -Wshadow to highlight.

The common "fix" to silence this warning -- renaming local y to something like y_ -- makes matters worse, because then it's much easier to confuse which is which and use the wrong one.

Yeah, sorry. My motivation behind this mad dash was that g++ was spewing so many compiler warnings that the travis logs were filling up and halting builds prematurely. In retrospect a better solution would have been to disable -Wshadow but instead I started "fixing" things before realizing how much of the code would need to be touched.

[springmeyer]

Noting that I'd also like to try to enable:

• -Wold-style-cast to catch places we are still using c style casts
• -Wfloat-equal to catch unsafe/non-portable floating point comparisons - we've got more of this stuff lurking: Avoid equality comparisons of floating point numbers node-mapnik#589
• Also interested if -Wexit-time-destructors -Wglobal-constructors -Wreserved-id-macro -Wheader-hygiene -Wmissing-noreturn have any value.