On Fri, Aug 17, 2012 at 3:54 PM, Jordan Rose <[email protected]> wrote: > > On Aug 17, 2012, at 15:43 , David Blaikie <[email protected]> wrote: > >> On Fri, Aug 17, 2012 at 3:33 PM, Nico Weber <[email protected]> wrote: >>> Should this really be on by default? On chrome, this triggers a single >>> time (linux-only): >>> >>> ../../third_party/tcmalloc/chromium/src/stack_trace_table.cc:138:16: >>> warning: expression which evaluates to zero treated as a null pointer >>> constant of type 'void *' [-Wnon-literal-null-conversion] >>> out[idx++] = static_cast<uintptr_t>(0); >>> ^~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> out is declared as `void** out = new void*[out_len];`. The warning >>> isn't wrong, but it looks rather pedantic to me. Should this be only >>> in -Wall (or maybe even in -pedantic)? >> >> Might be a fair candidate for -Wall, though it did find some >> reasonable stuff in google. 18 cases overall with some fairly >> interesting ones (see b/6954211 for the ones that've been committed so >> far, or cl/32692314 for some of the remaining ones. >> >> The worst offenders are integer constants with value 0 that aren't at >> all intended to be pointers. (most easily occurred in function calls >> where the caller thought the argument was of one type but it's >> actually of a pointer type) >> >> I have some more once this warning opens up to cover comparisons, >> conditional operands, and return statements - there's a lot of >> confusing "cstr == '\0'" code where the user probably meant to deref >> the lhs but didn't. > > IMHO, this should remain on by default. The Chromium example clearly shows an > impedance mismatch between the array and the value being stored. I would say > it's not unlikely that at one point the array was a uintptr_t*, but was > changed, and this part of the code wasn't updated to match because it didn't > warn. But I can see the argument that "because this isn't harmful, we > shouldn't warn unless asked to".
I don't feel strongly either way. This code is in one of the third-party libraries we use. We build those without -Wall because the warning policy is up to the library (not everybody believes in -Wall-clean), but we do build with the default warnings enabled so that clang can point out obvious bugs. It's easy for me to just disable this warning for the third-party library where it fires, but the warning felt like it's mostly pedantry. It sounds like it caught real bugs in google's internal code though, so *shrug* :-) Nico _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
