[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

[Nico Weber]

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* :-)

Nico