On Aug 19, 2012, at 4:10 PM, David Blaikie wrote: > On Sun, Aug 19, 2012 at 2:51 PM, John McCall <[email protected]> wrote: >> On Aug 19, 2012, at 2:38 PM, David Blaikie 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? >> >> Nothing in particular; I'm just saying that this is part of the mandate for >> this warning. >> >> static_cast<uintptr_t>(0) is actually not an unreasonable idiom: >> notably, it's actually portable across all compilers, platforms, and >> language dialects (assuming only the existence of the uintptr_t typedef), > > Fair point - I realize none of the forms I mentioned actually directly > produce the portability you described (just that they could be used to > produce it when wrapped in some macro-ness, or by relying on an > implementation detail (that NULL isn't just any null pointer, but an > appropriately sized one)).
Right. > Hmm - what happens if you pass nullptr via varargs? [expr.call]p7: An argument that has (possibly cv-qualified) type std::nullptr_t is converted to type void*. >> which none of the other idioms you've noted are. Of course, we can >> decide that it's uncommon enough to not be worth white-listing — >> a reasonable default assumption — but that's an empirical claim >> that can run up hard against reality. > > The only empirical evidence I can offer is that I've only seen that > idiom once (across google's codebase & including any 3rd party > libraries we use internally), and it appeared to be by accident rather > than any high-minded attempt to produce a pointer-sized null pointer > constant. Yeah, that's why I'm happy with the default stance of not white-listing it. I'm just saying that we need to keep our eyes open for this or others. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
