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

[Hans Wennborg via cfe-dev]

On Fri, Dec 22, 2017 at 2:59 AM, John McCall via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
>> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>>
>> 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?
>
> I think we should be more cautious and just treat it as an experimental warning like any other: that is, it should not be in any standard warning groups, even -Wextra.
>
> That is, for now, unless someone specifically passes -Wtautological-constant-compare (or -Weverything), they should just get the behavior prior to the implementation of this warning.  As Richard pointed out, this should only affect the new warning, not the existing -Wtautological-constant-out-of-range-compare.
>
> Don't feel bad about this; I think anyone who's done a significant amount of warning work has gone through the exact same thing.  I certainly know I have.  I think this is a very good diagnostic in principle, we just need to get it right.

Apologies for coming late to the thread.

What's the status here? Did everything land, and should we merge r321691 to 6.0?

Thanks,
Hans



>
> John.
>
>>
>>
>>
>> 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
>>>>
>>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev