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. -- James _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
