On Fri, Aug 17, 2012 at 4:00 PM, Nico Weber <[email protected]> wrote: > 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* :-)
FWIW, this seems a perfectly reasonable "on by default" warning to me, and I'm struggling to see the pedantry. I think many users would be surprised that static_cast<uintptr_t>(0) is a null pointer constant, and I doubt that its author meant it that way. I could be wrong. It's possible that the C++ standard will be revised so that this is no longer a NPC. In any case, code that triggers this warning is probably not as its author intended, and that sounds like a find time to issue a warning. -- James _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
