On Sun, Aug 19, 2012 at 9:14 PM, James Dennett <[email protected]> wrote: > On Sun, Aug 19, 2012 at 2:38 PM, David Blaikie <[email protected]> wrote: >> On Sun, Aug 19, 2012 at 2:35 PM, John McCall <[email protected]> wrote: >>> On Aug 17, 2012, at 10:16 PM, James Dennett wrote: >>>> 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. >>> >>> I agree; this should be on-by-default as long as we're properly >>> suppressing it in cases where the expression is a reasonable idiom >>> for creating a pointer-sized null constant. (Ensuring that a null constant >>> is pointer-sized is important when passing it to a variadic function). >> >> I believe NULL (which (well, GNUNull/__null does) seems to have the >> right target-dependent tweaks for size), nullptr, 0, and 0l, (0ul, >> 0u), etc should all work just fine. Did you have some other idiom(s) >> in mind for that particular purpose? > > Correct code will not use a null pointer constant in those contexts, > but rather pass a null pointer (e.g., static_cast<void*>(nullptr) or > static_cast<void*>(0)). > > That said, most code doesn't get this right, which is presumably why > implementations have historically defined NULL so that passing it via > varargs would happen to work in spite of this not being guaranteed by > the C or C++ standards.
Correcting myself (so quickly), nullptr is an exception -- that passes a void* as desired, though nullptr's type is not void*. Prior to C++11 code had to pass a pointer if it wanted standard-guaranteed behavior for a vararg function that expected a pointer argument. -- James _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
