Build with more aggressive compiler warnings

mapbox / tippecanoe

Build vector tilesets from large collections of GeoJSON features.

BSD 2-Clause "Simplified" License

2.71k stars 429 forks source link

Build with more aggressive compiler warnings #223

Open springmeyer opened 8 years ago

springmeyer commented 8 years ago

My sense is we should start with enabling and fixing warnings in roughly this order:

-Wabsolute-value
-Wfloat-conversion
-Wshorten-64-to-32
-Wconversion
-Wshadow
-Wsign-compare
-Wsign-conversion

e-n-f commented 8 years ago

-Wshadow should be fixed in f1b3f6d

springmeyer commented 6 years ago

@ericfischer - refreshed, recommended list is now available at https://github.com/mapbox/cpp/issues/37

e-n-f commented 6 years ago

Thanks. Working on this in https://github.com/mapbox/tippecanoe/pull/489

springmeyer commented 6 years ago

Thanks @ericfischer

springmeyer commented 6 years ago

@ericfischer great to see #489 land. Any progress on attempting -Wconversion or is it looking too narly?

e-n-f commented 6 years ago

That one is going to be terrible. There are hundreds of implicit type conversions.

springmeyer commented 6 years ago

@ericfischer okay, then I recommend starting with a subset like:

-Wbitfield-enum-conversion, -Wbool-conversion, -Wconstant-conversion, -Wenum-conversion, - -Wliteral-conversion, -Wnon-literal-null-conversion, -Wnull-conversion, -Wshorten-64-to-32, -Wstring-conversion.

I presume that will surface much fewer warnings, primarily from -Wshorten-64-to-32.

The whole -Wconversion group is listed at https://clang.llvm.org/docs/DiagnosticsReference.html#wconversion.

After those are resolved I recommend one-by-one incrementally adding -Wsign-conversion, then -Wint-conversion, and finally -Wfloat-conversion (which is often too much in my experience, but nevertheless useful to peek at even if you don't fix them).

springmeyer commented 6 years ago

@ericfischer do you have thoughts on my proposed plan?

e-n-f commented 6 years ago

I can certainly go through and add 571 type casts. I'm just not sure what the balance will turn out to be between:

fixing genuine bugs
adding needless clutter
accidentally masking genuine bugs by adding casts that make errors look intentional

which is always my anxiety around casts. I hope there will be more of the former than of the latter two though.

e-n-f commented 6 years ago

The fundamental tension is that the language really wants array bounds and indices to be unsigned, for efficiency, but really wants ordinary arithmetic to be signed, for intuitive results.

e-n-f commented 6 years ago

Trying to get more rigorous about numeric types in https://github.com/mapbox/tippecanoe/pull/506.

Situations I've encountered so far that are stuck as int but perhaps should not be:

Tile coordinate detail (log2(extent))
Global scaling factor for tile coordinates
Feature flags (because that's what getopt_long wants)
Comparator return values (because that's what qsort wants)
Zoom levels
File descriptors (because that's what open et al want)
Attribute types (for coercion)
Exit status
Char+EOF return values from getc
Char+EOF return from getopt_long, and optind option index

springmeyer commented 6 years ago

Excellent progress @ericfischer - I notice that branch fixes a number of the -Wshorten-64-to-32 warnings. Some that remain are coming from the zlib wrapper code, which reminds me that these warnings are solved in https://github.com/mapbox/gzip-hpp, which is fundamentally a port of the same code in tippecanoe into a standalone module with the warnings (and potential overflows) resolved.

e-n-f commented 6 years ago

What remains now is a handful of int-to-char conversions and hundreds of sign conversions.