[cfe-dev] -Wtautological-constant-compare issues

[Roman Lebedev via cfe-dev]

Hi all!

First of all, i'm sorry for creating this issue, it was not my intention :)

I have created https://reviews.llvm.org/D41512
Does it address all the concerns without completely killing the diagnostic?



While there, i have a question about line 8870 of semachecking.cpp:
https://github.com/llvm-mirror/clang/blob/c98d5adb4e64815effa4855a3804cc1a9a7d2958/lib/Sema/SemaChecking.cpp#L8870

if (InRange && IsEnumConstOrFromMacro(S, Constant))
  return false;

This completely prevents the diagnostic from firing on cases like:

  unsigned char x = ...
  assert(x <= 255);

https://godbolt.org/g/dYx96F

Is this *really* the intention here?
Perhaps it was meant to silence in in the cases like:

  #define val_is_in_bounds(x) ((x) <= 255)
  ...
  unsigned char x = ...
  assert(val_is_in_bounds(x));

?


Roman.

On Thu, Dec 21, 2017 at 12:08 AM, Reid Kleckner <rnk at google.com> wrote:
> +Roman, the patch author
>
> +1 for disabling this, most of these have been false positives.
>
> On Tue, Dec 19, 2017 at 3:02 PM, Shoaib Meenai via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>>
>> Hi all (CC some people who have been involved in this discussion already
>> and Hans for clang 6 release discussion),
>>
>>
>>
>> -Wtautological-constant-compare was introduced in r315614 to diagnose
>> tautological comparisons against a type's maximum and minimum bounds.
>> However, this warning can fire spuriously when a type's size is
>> platform-dependent. For example, libc++ has some generic code for which the
>> warning fires on platforms where int and long have the same size; see
>> https://reviews.llvm.org/D39149 for my initial attempt to work around the
>> warning, and then https://reviews.llvm.org/D41368, which just silences the
>> warning where it's problematic.
>>
>>
>>
>> Unfortunately, this appears to be a pretty widespread problem. For
>> example, Petr Hosek reported in https://reviews.llvm.org/D39462 that he had
>> to globally disable the warning when they rolled out a newer clang. Roman
>> attempted to address this problem by having the warning take type size
>> differences into account in https://reviews.llvm.org/D39462, but that's
>> tricky to implement and hit some implementation roadblocks.
>>
>>
>>
>> I think shipping the warning in its current state in clang 6 is going to
>> be problematic, because there are going to be many instances of generic code
>> running into spurious warnings. At the very least, I think the warning as-is
>> shouldn't be part of -Wall. What are other people's thoughts on this issue?
>>
>>
>>
>> Thanks,
>>
>> Shoaib
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>