r210372 - Add -Wtautological-undefined-compare and -Wundefined-bool-conversion warnings

[Nico Weber]

On Tue, Aug 5, 2014 at 3:43 PM, Chandler Carruth <chandlerc at google.com>
wrote:

>
> On Tue, Aug 5, 2014 at 3:36 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> This is a very useful warning, thanks.
>>
>> One issue I have with it is that it's disabled
>> by Wno-tautological-compare. From the warning name that makes sense –
>> but Wtautological-compare used to be a fairly harmless warning, so we
>> (chromium) disabled it for a bunch of third-party libraries. When
>> Wtautological-undefined-compare arrived, we fixed all instances of this in
>> our code and updated our compiler to a clang that optimizes away
>> comparisons of references with NULL (since we had fixed all of these
>> comparisons, we thought!)
>>
>> I recently realized that we didn't see Wtautological-undefined-compare
>> warnings for all the third-party libraries that we're building with
>> Wno-tautological-compare.
>>
>> Do you think Wtautological-undefined-compare should be in its own warning
>> group (and maybe have a different name to make that clear), so that folks
>> who disabled Wtautological-compare still get this warning? The reasoning is
>> that this warning is much more serious that what Wtautological-compare used
>> to warn about.
>>
>
> I don't really understand why. There is no actual undefined behavior here,
> just a comparison that can never, ever be go the other way. It seems almost
> exactly as severe as comparing an unsigned number for >= 0?
>

The undefined behavior isn't the check, but the code that preceded it and
that made someone put in the fix:

  int* a = nullptr;
  int& ra = *a;  // Undefined! But also harmless in practice.

  if (a == 5) {} // Crash! Better "fix" this by instead writing:
  if (&a && a == 5) {} // No crash! Except if the compiler rightfully
optimizes away the &a check.

If you don't have an ubsan bot, then it's possible that your code has
references to null. That's undefined, but used to be safe in practice – but
now that clang marks addresses of references as non-null, it isn't
anymore. Wtautological-undefined-compare catches some of these places.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140805/1c0916da/attachment.html>