[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 9:14 PM, James Dennett <jdennett at google.com> wrote:
> 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.

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