[cfe-commits] r161501 - in /cfe/trunk: include/clang/AST/Expr.h include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/AST/Expr.cpp lib/Sema/SemaExpr.cpp lib/Sema/SemaOverload.cpp test/SemaCXX/constant-expression-c

[John McCall]

On Aug 19, 2012, at 4:10 PM, David Blaikie wrote:
> On Sun, Aug 19, 2012 at 2:51 PM, John McCall <rjmccall at apple.com> wrote:
>> On Aug 19, 2012, at 2:38 PM, David Blaikie wrote:
>>> On Sun, Aug 19, 2012 at 2:35 PM, John McCall <rjmccall at apple.com> wrote:
>>>> On Aug 17, 2012, at 10:16 PM, James Dennett wrote:
>>>>> On Fri, Aug 17, 2012 at 4:00 PM, Nico Weber <thakis at chromium.org> wrote:
>>>>>> On Fri, Aug 17, 2012 at 3:54 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>>>>> 
>>>>>>> On Aug 17, 2012, at 15:43 , David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>> 
>>>>>>>> On Fri, Aug 17, 2012 at 3:33 PM, Nico Weber <thakis at chromium.org> 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.