-Wtautological-constant-compare issues

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

-Wtautological-constant-compare issues

Eric Fiselier via cfe-dev

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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
Hi,

On Dec 19, 2017, at 3:02 PM, Shoaib Meenai via cfe-dev <[hidden email]> 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;

I think our kernel team is planning on disabling -Wtautological-constant-compare for this exact reason. I've CC'd Alex, who may know more about any issues with this diagnostic w.r.t Apple projects. If the false positive really is widespread, I'd be supportive of disabling it by default or in -Wall.

vedant


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev

On Dec 19, 2017, at 6:02 PM, Shoaib Meenai <[hidden email]> 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; seehttps://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?

If there's significant community concern about this, and to me that standard has been met, then I think the only reasonable solution is to take it out of -Wall until we feel more confident in it.

John.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On 20 Dec 2017 03:51, "John McCall via cfe-dev" <[hidden email]> wrote:

On Dec 19, 2017, at 6:02 PM, Shoaib Meenai <[hidden email]> 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; seehttps://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?

If there's significant community concern about this, and to me that standard has been met, then I think the only reasonable solution is to take it out of -Wall until we feel more confident in it.

I agree. I think we currently have -Wtautological-out-of-range-constant-compare (or similar? not near a computer right now...) as a subgroup; that should remain in -Wall as it predates this new warning, and we should make sure we have a dedicated flag for just the "in the range of the type" portion of the warning.

John.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
On Tue, Dec 19, 2017 at 7:51 PM, John McCall <[hidden email]> wrote:

On Dec 19, 2017, at 6:02 PM, Shoaib Meenai <[hidden email]> 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; seehttps://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?

If there's significant community concern about this, and to me that standard has been met, then I think the only reasonable solution is to take it out of -Wall until we feel more confident in it.


I concur.

Yesterday I received the following bug report against libc++:
   Bug 35698 - include/istream causes test failures on x86 due to -Werror,-Wtautological-constant-compare
   https://bugs.llvm.org/show_bug.cgi?id=35698

-- Marshall


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
+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 <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier 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 <[hidden email]> 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
> <[hidden email]> 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
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev

> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <[hidden email]> 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.

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Fri, Dec 22, 2017 at 2:59 AM, John McCall via cfe-dev
<[hidden email]> wrote:

>
>> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <[hidden email]> 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 <[hidden email]> 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
>>> <[hidden email]> 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
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Tue, Jan 16, 2018 at 8:42 AM, Hans Wennborg via cfe-dev
<[hidden email]> wrote:

> On Fri, Dec 22, 2017 at 2:59 AM, John McCall via cfe-dev
> <[hidden email]> wrote:
>>
>>> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <[hidden email]> 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?

It landed and I think we should merge r321691 to 6.0.

~Aaron
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
On Tue, Jan 16, 2018 at 4:42 PM, Hans Wennborg <[hidden email]> wrote:

> On Fri, Dec 22, 2017 at 2:59 AM, John McCall via cfe-dev
> <[hidden email]> wrote:
>>
>>> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <[hidden email]> 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.

> 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 <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>> [hidden email]
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>
>>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Tue, Jan 16, 2018 at 2:51 PM, Roman Lebedev <[hidden email]> wrote:

> On Tue, Jan 16, 2018 at 4:42 PM, Hans Wennborg <[hidden email]> wrote:
>> On Fri, Dec 22, 2017 at 2:59 AM, John McCall via cfe-dev
>> <[hidden email]> wrote:
>>>
>>>> On Dec 21, 2017, at 4:28 PM, Roman Lebedev via cfe-dev <[hidden email]> 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 <[hidden email]> 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
>>>>> <[hidden email]> 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
>>>>>> [hidden email]
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>>
>>>>>
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
> Wennborg via cfe-dev
> Sent: Tuesday, January 16, 2018 8:01 AM
> To: Roman Lebedev <[hidden email]>
> Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
> [hidden email]>; [hidden email]; John McCall
> <[hidden email]>
> Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>
...

> >>
> >> 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.
>

Can we consider https://reviews.llvm.org/D41727 for inclusion in master and possibly release_60?

-Brian

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
Wait, wasn't the consensus here to leave the warning out of -Wextra too? Looks like r321691 got that wrong?

On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]> wrote:
> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
> Wennborg via cfe-dev
> Sent: Tuesday, January 16, 2018 8:01 AM
> To: Roman Lebedev <[hidden email]>
> Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
> [hidden email]>; [hidden email]; John McCall
> <[hidden email]>
> Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>
...
> >>
> >> 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.
>

Can we consider https://reviews.llvm.org/D41727 for inclusion in master and possibly release_60?

-Brian

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6. Hopefully we can find good heuristics to suppress the false positives for Clang 7 and then re-enable it.

On 16 January 2018 at 09:35, Nico Weber via cfe-dev <[hidden email]> wrote:
Wait, wasn't the consensus here to leave the warning out of -Wextra too? Looks like r321691 got that wrong?

On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]> wrote:
> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
> Wennborg via cfe-dev
> Sent: Tuesday, January 16, 2018 8:01 AM
> To: Roman Lebedev <[hidden email]>
> Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
> [hidden email]>; [hidden email]; John McCall
> <[hidden email]>
> Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>
...
> >>
> >> 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.
>

Can we consider https://reviews.llvm.org/D41727 for inclusion in master and possibly release_60?

-Brian

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
On Tue, Jan 16, 2018 at 8:35 PM, Nico Weber <[hidden email]> wrote:
> Wait, wasn't the consensus here to leave the warning out of -Wextra too?
> Looks like r321691 got that wrong?
Hm, consensus? Since it was pretty quite clear (?) from the diff and commit
message that it was only moved to -Wextra, i guess this was somehow
missed by me and all the reviewers...

On Tue, Jan 16, 2018 at 8:38 PM, Richard Smith via cfe-dev
<[hidden email]> wrote:
> Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6.
What exactly do you have in mind?
Only from release_60 branch, or from trunk+release_60?

> Hopefully we can find good heuristics to suppress the false positives for
> Clang 7 and then re-enable it.
John McCall did post a short summary of what *seemingly* needs to be
done in https://reviews.llvm.org/D39462#917421
To be honest i'm not quite sure where to start on that.

Roman.

> On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]>
> wrote:
>>
>> > -----Original Message-----
>> > From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
>> > Wennborg via cfe-dev
>> > Sent: Tuesday, January 16, 2018 8:01 AM
>> > To: Roman Lebedev <[hidden email]>
>> > Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
>> > [hidden email]>; [hidden email]; John McCall
>> > <[hidden email]>
>> > Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>> >
>> ...
>> > >>
>> > >> 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.
>> >
>>
>> Can we consider https://reviews.llvm.org/D41727 for inclusion in master
>> and possibly release_60?
>>
>> -Brian
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Tue, Jan 16, 2018 at 12:55 PM, Roman Lebedev <[hidden email]> wrote:
On Tue, Jan 16, 2018 at 8:35 PM, Nico Weber <[hidden email]> wrote:
> Wait, wasn't the consensus here to leave the warning out of -Wextra too?
> Looks like r321691 got that wrong?
Hm, consensus? Since it was pretty quite clear (?) from the diff and commit
message that it was only moved to -Wextra, i guess this was somehow
missed by me and all the reviewers...

On Tue, Jan 16, 2018 at 8:38 PM, Richard Smith via cfe-dev
<[hidden email]> wrote:
> Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6.
What exactly do you have in mind?
Only from release_60 branch, or from trunk+release_60?

trunk+release_60; we do trunk-based development.
 

> Hopefully we can find good heuristics to suppress the false positives for
> Clang 7 and then re-enable it.
John McCall did post a short summary of what *seemingly* needs to be
done in https://reviews.llvm.org/D39462#917421
To be honest i'm not quite sure where to start on that.

Roman.

> On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]>
> wrote:
>>
>> > -----Original Message-----
>> > From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
>> > Wennborg via cfe-dev
>> > Sent: Tuesday, January 16, 2018 8:01 AM
>> > To: Roman Lebedev <[hidden email]>
>> > Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
>> > [hidden email]>; [hidden email]; John McCall
>> > <[hidden email]>
>> > Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>> >
>> ...
>> > >>
>> > >> 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.
>> >
>>
>> Can we consider https://reviews.llvm.org/D41727 for inclusion in master
>> and possibly release_60?
>>
>> -Brian
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
Roman, do you want to remove the warning from -Wextra, or do you want me to do it?

On Tue, Jan 16, 2018 at 12:59 PM, Nico Weber <[hidden email]> wrote:
On Tue, Jan 16, 2018 at 12:55 PM, Roman Lebedev <[hidden email]> wrote:
On Tue, Jan 16, 2018 at 8:35 PM, Nico Weber <[hidden email]> wrote:
> Wait, wasn't the consensus here to leave the warning out of -Wextra too?
> Looks like r321691 got that wrong?
Hm, consensus? Since it was pretty quite clear (?) from the diff and commit
message that it was only moved to -Wextra, i guess this was somehow
missed by me and all the reviewers...

On Tue, Jan 16, 2018 at 8:38 PM, Richard Smith via cfe-dev
<[hidden email]> wrote:
> Yes, I'd prefer to take this warning out of -Wextra at least for Clang 6.
What exactly do you have in mind?
Only from release_60 branch, or from trunk+release_60?

trunk+release_60; we do trunk-based development.
 

> Hopefully we can find good heuristics to suppress the false positives for
> Clang 7 and then re-enable it.
John McCall did post a short summary of what *seemingly* needs to be
done in https://reviews.llvm.org/D39462#917421
To be honest i'm not quite sure where to start on that.

Roman.

> On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]>
> wrote:
>>
>> > -----Original Message-----
>> > From: cfe-dev [mailto:[hidden email]] On Behalf Of Hans
>> > Wennborg via cfe-dev
>> > Sent: Tuesday, January 16, 2018 8:01 AM
>> > To: Roman Lebedev <[hidden email]>
>> > Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
>> > [hidden email]>; [hidden email]; John McCall
>> > <[hidden email]>
>> > Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>> >
>> ...
>> > >>
>> > >> 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.
>> >
>>
>> Can we consider https://reviews.llvm.org/D41727 for inclusion in master
>> and possibly release_60?
>>
>> -Brian
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Wed, Jan 17, 2018 at 6:25 PM, Nico Weber <[hidden email]> wrote:
> Roman, do you want to remove the warning from -Wextra
No, i do not want to remove the warning from -Wextra.
I have specified that quite clearly in all previous communications.

Next time, please do actually review such differentials, if you care.
Much better than having this disscussion this late after branching.

> or do you want me to do it?
So be it. *Please* do spell all these details in the commit message.

Roman.

> On Tue, Jan 16, 2018 at 12:59 PM, Nico Weber <[hidden email]> wrote:
>>
>> On Tue, Jan 16, 2018 at 12:55 PM, Roman Lebedev <[hidden email]>
>> wrote:
>>>
>>> On Tue, Jan 16, 2018 at 8:35 PM, Nico Weber <[hidden email]> wrote:
>>> > Wait, wasn't the consensus here to leave the warning out of -Wextra
>>> > too?
>>> > Looks like r321691 got that wrong?
>>> Hm, consensus? Since it was pretty quite clear (?) from the diff and
>>> commit
>>> message that it was only moved to -Wextra, i guess this was somehow
>>> missed by me and all the reviewers...
>>>
>>> On Tue, Jan 16, 2018 at 8:38 PM, Richard Smith via cfe-dev
>>> <[hidden email]> wrote:
>>> > Yes, I'd prefer to take this warning out of -Wextra at least for Clang
>>> > 6.
>>> What exactly do you have in mind?
>>> Only from release_60 branch, or from trunk+release_60?
>>
>>
>> trunk+release_60; we do trunk-based development.
>>
>>>
>>>
>>> > Hopefully we can find good heuristics to suppress the false positives
>>> > for
>>> > Clang 7 and then re-enable it.
>>> John McCall did post a short summary of what *seemingly* needs to be
>>> done in https://reviews.llvm.org/D39462#917421
>>> To be honest i'm not quite sure where to start on that.
>>>
>>> Roman.
>>>
>>> > On Tue, Jan 16, 2018 at 12:04 PM, via cfe-dev <[hidden email]>
>>> > wrote:
>>> >>
>>> >> > -----Original Message-----
>>> >> > From: cfe-dev [mailto:[hidden email]] On Behalf Of
>>> >> > Hans
>>> >> > Wennborg via cfe-dev
>>> >> > Sent: Tuesday, January 16, 2018 8:01 AM
>>> >> > To: Roman Lebedev <[hidden email]>
>>> >> > Cc: Marshall Clow <[hidden email]>; Richard Smith <richard-
>>> >> > [hidden email]>; [hidden email]; John McCall
>>> >> > <[hidden email]>
>>> >> > Subject: Re: [cfe-dev] -Wtautological-constant-compare issues
>>> >> >
>>> >> ...
>>> >> > >>
>>> >> > >> 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.
>>> >> >
>>> >>
>>> >> Can we consider https://reviews.llvm.org/D41727 for inclusion in
>>> >> master
>>> >> and possibly release_60?
>>> >>
>>> >> -Brian
>>> >>
>>> >> _______________________________________________
>>> >> cfe-dev mailing list
>>> >> [hidden email]
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> >
>>> >
>>
>>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wtautological-constant-compare issues

Eric Fiselier via cfe-dev
On Wed, Jan 17, 2018 at 7:32 AM, Roman Lebedev <[hidden email]> wrote:
On Wed, Jan 17, 2018 at 6:25 PM, Nico Weber <[hidden email]> wrote:
> Roman, do you want to remove the warning from -Wextra
No, i do not want to remove the warning from -Wextra.
I have specified that quite clearly in all previous communications.

Next time, please do actually review such differentials, if you care.
Much better than having this disscussion this late after branching.

> or do you want me to do it?
So be it. *Please* do spell all these details in the commit message.


I just received https://bugs.llvm.org/show_bug.cgi?id=35997, which was reported by a user building with '-Wall', '-Wextra', '-Werror'.

-- Marshall


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12