[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

[James Dennett]

On Sun, Aug 19, 2012 at 2:38 PM, David Blaikie <dblaikie at gmail.com> 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?

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