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

Tue Jan 16 06:01:13 PST 2018

On Tue, Jan 16, 2018 at 2:51 PM, Roman Lebedev <lebedev.ri at gmail.com> wrote:
> On Tue, Jan 16, 2018 at 4:42 PM, Hans Wennborg <hans at chromium.org> wrote:
>> 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.
> No problem.
>
>> What's the status here? Did everything land, and should we merge r321691 to 6.0?
> It landed before branching. I see r321691 in release_60 branch.
> So nothing do be done here.

Ah, right. Thanks for checking! Sorry for the noise.

>
>> Thanks,
>> Hans
> Roman.
>
>>>
>>> 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