Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Roman Lebedev via cfe-commits]

On Fri, Jan 19, 2018 at 2:22 AM, Alex L  wrote:
> Hi Roman,
Hi.

> This commit has caused a regression in LLVM 6 which now triggers
> -Wsign-compare for typeof(enum) and typeof(enumConstant).
Interesting, first impression is that it appears to be a false-positive.

> I filed
> https://bugs.llvm.org/show_bug.cgi?id=36008. Could you please take a look at
> it?
Will do, thanks.

> Thanks,
> Alex
Roman.

> On 21 October 2017 at 09:44, Roman Lebedev via cfe-commits
>  wrote:
>>
>> Author: lebedevri
>> Date: Sat Oct 21 09:44:03 2017
>> New Revision: 316268
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
>> Log:
>> [Sema] Fixes for enum handling for tautological comparison diagnostics
>>
>> Summary:
>> As Mattias Eriksson has reported in PR35009, in C, for enums, the
>> underlying type should
>> be used when checking for the tautological comparison, unlike C++, where
>> the enumerator
>> values define the value range. So if not in CPlusPlus mode, use the enum
>> underlying type.
>>
>> Also, i have discovered a problem (a crash) when evaluating
>> tautological-ness of the following comparison:
>> ```
>> enum A { A_a = 0 };
>> if (a < 0) // expected-warning {{comparison of unsigned enum expression <
>> 0 is always false}}
>> return 0;
>> ```
>> This affects both the C and C++, but after the first fix, only C++ code
>> was affected.
>> That was also fixed, while preserving (i think?) the proper diagnostic
>> output.
>>
>> And while there, attempt to enhance the test coverage.
>> Yes, some tests got moved around, sorry about that :)
>>
>> Fixes PR35009
>>
>> Reviewers: aaron.ballman, rsmith, rjmccall
>>
>> Reviewed By: aaron.ballman
>>
>> Subscribers: Rakete, efriedma, materi, cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D39122
>>
>> Added:
>> cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
>> cfe/trunk/test/Sema/tautological-constant-enum-compare.c
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
>> @@ -8181,8 +8181,12 @@ struct IntRange {
>>  if (const AtomicType *AT = dyn_cast(T))
>>T = AT->getValueType().getTypePtr();
>>
>> -// For enum types, use the known bit width of the enumerators.
>> -if (const EnumType *ET = dyn_cast(T)) {
>> +if (!C.getLangOpts().CPlusPlus) {
>> +  // For enum types in C code, use the underlying datatype.
>> +  if (const EnumType *ET = dyn_cast(T))
>> +T =
>> ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
>> +} else if (const EnumType *ET = dyn_cast(T)) {
>> +  // For enum types in C++, use the known bit width of the
>> enumerators.
>>EnumDecl *Enum = ET->getDecl();
>>// In C++11, enums without definitions can have an explicitly
>> specified
>>// underlying type.  Use this type to compute the range.
>> @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
>>  }
>>
>>  enum class LimitType {
>> -  Max, // e.g. 32767 for short
>> -  Min  // e.g. -32768 for short
>> +  Max = 1U << 0U,  // e.g. 32767 for short
>> +  Min = 1U << 1U,  // e.g. -32768 for short
>> +  Both = Max | Min // When the value is both the Min and the Max limit at
>> the
>> +   // same time; e.g. in C++, A::a in enum A { a = 0 };
>>  };
>>
>>  /// Checks whether Expr 'Constant' may be the
>> @@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
>>
>>IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>>
>> +  // Special-case for C++ for enum with one enumerator with value of 0.
>> +  if (OtherRange.Width == 0)
>> +return Value == 0 ? LimitType::Both : llvm::Optional();
>> +
>>if (llvm::APSInt::isSameValue(
>>llvm::APSInt::getMaxValue(OtherRange.Width,
>>  OtherT->isUnsignedIntegerType()),
>> @@ -8620,7 +8630,7 @@ llvm::Optional IsTypeLimit(Se
>>Value))
>>  return LimitType::Min;
>>
>> -  return llvm::Optional();
>> +  return llvm::None;
>>  }
>>
>>  bool HasEnumType(Expr *E) {
>> @@ -8655,9 +8665,12 @@ bool CheckTautologicalComparison(Sema 
>>
>>bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
>>bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
>> -  bool ResultWhenConstNeOther =
>> -  ConstIsLowerBound ^ (ValueType == LimitType::Max);
>> -  if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
>> +  if (ValueType != LimitType::Both) {
>> +   

Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Alex L via cfe-commits]

Hi Roman,

This commit has caused a regression in LLVM 6 which now triggers
-Wsign-compare for typeof(enum) and typeof(enumConstant). I filed
https://bugs.llvm.org/show_bug.cgi?id=36008. Could you please take a look
at it?

Thanks,
Alex

On 21 October 2017 at 09:44, Roman Lebedev via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: lebedevri
> Date: Sat Oct 21 09:44:03 2017
> New Revision: 316268
>
> URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
> Log:
> [Sema] Fixes for enum handling for tautological comparison diagnostics
>
> Summary:
> As Mattias Eriksson has reported in PR35009, in C, for enums, the
> underlying type should
> be used when checking for the tautological comparison, unlike C++, where
> the enumerator
> values define the value range. So if not in CPlusPlus mode, use the enum
> underlying type.
>
> Also, i have discovered a problem (a crash) when evaluating
> tautological-ness of the following comparison:
> ```
> enum A { A_a = 0 };
> if (a < 0) // expected-warning {{comparison of unsigned enum expression <
> 0 is always false}}
> return 0;
> ```
> This affects both the C and C++, but after the first fix, only C++ code
> was affected.
> That was also fixed, while preserving (i think?) the proper diagnostic
> output.
>
> And while there, attempt to enhance the test coverage.
> Yes, some tests got moved around, sorry about that :)
>
> Fixes PR35009
>
> Reviewers: aaron.ballman, rsmith, rjmccall
>
> Reviewed By: aaron.ballman
>
> Subscribers: Rakete, efriedma, materi, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D39122
>
> Added:
> cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
> cfe/trunk/test/Sema/tautological-constant-enum-compare.c
> Modified:
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
> SemaChecking.cpp?rev=316268=316267=316268=diff
> 
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
> @@ -8181,8 +8181,12 @@ struct IntRange {
>  if (const AtomicType *AT = dyn_cast(T))
>T = AT->getValueType().getTypePtr();
>
> -// For enum types, use the known bit width of the enumerators.
> -if (const EnumType *ET = dyn_cast(T)) {
> +if (!C.getLangOpts().CPlusPlus) {
> +  // For enum types in C code, use the underlying datatype.
> +  if (const EnumType *ET = dyn_cast(T))
> +T = ET->getDecl()->getIntegerType().getDesugaredType(C).
> getTypePtr();
> +} else if (const EnumType *ET = dyn_cast(T)) {
> +  // For enum types in C++, use the known bit width of the
> enumerators.
>EnumDecl *Enum = ET->getDecl();
>// In C++11, enums without definitions can have an explicitly
> specified
>// underlying type.  Use this type to compute the range.
> @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
>  }
>
>  enum class LimitType {
> -  Max, // e.g. 32767 for short
> -  Min  // e.g. -32768 for short
> +  Max = 1U << 0U,  // e.g. 32767 for short
> +  Min = 1U << 1U,  // e.g. -32768 for short
> +  Both = Max | Min // When the value is both the Min and the Max limit at
> the
> +   // same time; e.g. in C++, A::a in enum A { a = 0 };
>  };
>
>  /// Checks whether Expr 'Constant' may be the
> @@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
>
>IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>
> +  // Special-case for C++ for enum with one enumerator with value of 0.
> +  if (OtherRange.Width == 0)
> +return Value == 0 ? LimitType::Both : llvm::Optional();
> +
>if (llvm::APSInt::isSameValue(
>llvm::APSInt::getMaxValue(OtherRange.Width,
>  OtherT->isUnsignedIntegerType()),
> @@ -8620,7 +8630,7 @@ llvm::Optional IsTypeLimit(Se
>Value))
>  return LimitType::Min;
>
> -  return llvm::Optional();
> +  return llvm::None;
>  }
>
>  bool HasEnumType(Expr *E) {
> @@ -8655,9 +8665,12 @@ bool CheckTautologicalComparison(Sema 
>
>bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
>bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
> -  bool ResultWhenConstNeOther =
> -  ConstIsLowerBound ^ (ValueType == LimitType::Max);
> -  if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
> +  if (ValueType != LimitType::Both) {
> +bool ResultWhenConstNeOther =
> +ConstIsLowerBound ^ (ValueType == LimitType::Max);
> +if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
> +  return false; // The comparison is not tautological.
> +  } else if (ResultWhenConstEqualsOther == ConstIsLowerBound)
>  return false; 

Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Roman Lebedev via cfe-commits]

On Tue, Oct 24, 2017 at 3:00 PM, Hans Wennborg  wrote:
> On Mon, Oct 23, 2017 at 2:02 PM, Roman Lebedev  wrote:
>> On Mon, Oct 23, 2017 at 2:13 PM, Hans Wennborg  wrote:
>> Hi.
>>
>>> This seems to have had the side effect of introducing a new warning in
>>> Chromium builds:
>>>
>>> ../../third_party/expat/files/lib/xmlparse.c(2429,24):  error:
>>> comparison of integers of different signs: 'enum XML_Error' and
>>> 'unsigned int' [-Werror,-Wsign-compare]
>>>   if (code > 0 && code < sizeof(message)/sizeof(message[0]))
>>>    ^ ~~
>> (I guess this is on windows)
>
> Yes.
>
>>
>>> I'm not sure if this was intentional or not.
>>>
>>> The warning seems technically correct here, though not very useful in
>>> this specific case.
>> I *believe* that was caused by the fix for 
>> IntRange::forValueOfCanonicalType()
>> to use enum's underlying type for C code. That fix was absolutely 
>> intentional.
>>
>> However that new -Wsign-compare diagnostic (and i suspect there may be
>> more repercussions) was not really intentional.
>> However as you have said, and i think i agree, the diagnostic valid.
>>
>> So perhaps i simply should add a test and release notes entry?
>
> Sounds reasonable. It turns out we only got this single extra
> -Wsign-compare warning, so it didn't turn out to be a big problem for
> us.

Ok, committed in https://reviews.llvm.org/rL316500
Roman.

>>
>> Roman.
>>
>>> On Sat, Oct 21, 2017 at 6:44 PM, Roman Lebedev via cfe-commits
>>>  wrote:
 Author: lebedevri
 Date: Sat Oct 21 09:44:03 2017
 New Revision: 316268

 URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
 Log:
 [Sema] Fixes for enum handling for tautological comparison diagnostics

 Summary:
 As Mattias Eriksson has reported in PR35009, in C, for enums, the 
 underlying type should
 be used when checking for the tautological comparison, unlike C++, where 
 the enumerator
 values define the value range. So if not in CPlusPlus mode, use the enum 
 underlying type.

 Also, i have discovered a problem (a crash) when evaluating 
 tautological-ness of the following comparison:
 ```
 enum A { A_a = 0 };
 if (a < 0) // expected-warning {{comparison of unsigned enum expression < 
 0 is always false}}
 return 0;
 ```
 This affects both the C and C++, but after the first fix, only C++ code 
 was affected.
 That was also fixed, while preserving (i think?) the proper diagnostic 
 output.

 And while there, attempt to enhance the test coverage.
 Yes, some tests got moved around, sorry about that :)

 Fixes PR35009

 Reviewers: aaron.ballman, rsmith, rjmccall

 Reviewed By: aaron.ballman

 Subscribers: Rakete, efriedma, materi, cfe-commits

 Tags: #clang

 Differential Revision: https://reviews.llvm.org/D39122

 Added:
 cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
 cfe/trunk/test/Sema/tautological-constant-enum-compare.c
 Modified:
 cfe/trunk/lib/Sema/SemaChecking.cpp
 cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
 cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp

 Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
 URL: 
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
 ==
 --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
 @@ -8181,8 +8181,12 @@ struct IntRange {
  if (const AtomicType *AT = dyn_cast(T))
T = AT->getValueType().getTypePtr();

 -// For enum types, use the known bit width of the enumerators.
 -if (const EnumType *ET = dyn_cast(T)) {
 +if (!C.getLangOpts().CPlusPlus) {
 +  // For enum types in C code, use the underlying datatype.
 +  if (const EnumType *ET = dyn_cast(T))
 +T = 
 ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
 +} else if (const EnumType *ET = dyn_cast(T)) {
 +  // For enum types in C++, use the known bit width of the 
 enumerators.
EnumDecl *Enum = ET->getDecl();
// In C++11, enums without definitions can have an explicitly 
 specified
// underlying type.  Use this type to compute the range.
 @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
  }

  enum class LimitType {
 -  Max, // e.g. 32767 for short
 -  Min  // e.g. -32768 for short
 +  Max = 1U << 0U,  // e.g. 32767 for short
 +  Min = 1U << 1U,  // e.g. -32768 for short
 +  Both = Max | Min // When the value is both the Min and the Max 

Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Hans Wennborg via cfe-commits]

On Mon, Oct 23, 2017 at 2:02 PM, Roman Lebedev  wrote:
> On Mon, Oct 23, 2017 at 2:13 PM, Hans Wennborg  wrote:
> Hi.
>
>> This seems to have had the side effect of introducing a new warning in
>> Chromium builds:
>>
>> ../../third_party/expat/files/lib/xmlparse.c(2429,24):  error:
>> comparison of integers of different signs: 'enum XML_Error' and
>> 'unsigned int' [-Werror,-Wsign-compare]
>>   if (code > 0 && code < sizeof(message)/sizeof(message[0]))
>>    ^ ~~
> (I guess this is on windows)

Yes.

>
>> I'm not sure if this was intentional or not.
>>
>> The warning seems technically correct here, though not very useful in
>> this specific case.
> I *believe* that was caused by the fix for IntRange::forValueOfCanonicalType()
> to use enum's underlying type for C code. That fix was absolutely intentional.
>
> However that new -Wsign-compare diagnostic (and i suspect there may be
> more repercussions) was not really intentional.
> However as you have said, and i think i agree, the diagnostic valid.
>
> So perhaps i simply should add a test and release notes entry?

Sounds reasonable. It turns out we only got this single extra
-Wsign-compare warning, so it didn't turn out to be a big problem for
us.

>
> Roman.
>
>> On Sat, Oct 21, 2017 at 6:44 PM, Roman Lebedev via cfe-commits
>>  wrote:
>>> Author: lebedevri
>>> Date: Sat Oct 21 09:44:03 2017
>>> New Revision: 316268
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
>>> Log:
>>> [Sema] Fixes for enum handling for tautological comparison diagnostics
>>>
>>> Summary:
>>> As Mattias Eriksson has reported in PR35009, in C, for enums, the 
>>> underlying type should
>>> be used when checking for the tautological comparison, unlike C++, where 
>>> the enumerator
>>> values define the value range. So if not in CPlusPlus mode, use the enum 
>>> underlying type.
>>>
>>> Also, i have discovered a problem (a crash) when evaluating 
>>> tautological-ness of the following comparison:
>>> ```
>>> enum A { A_a = 0 };
>>> if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 
>>> is always false}}
>>> return 0;
>>> ```
>>> This affects both the C and C++, but after the first fix, only C++ code was 
>>> affected.
>>> That was also fixed, while preserving (i think?) the proper diagnostic 
>>> output.
>>>
>>> And while there, attempt to enhance the test coverage.
>>> Yes, some tests got moved around, sorry about that :)
>>>
>>> Fixes PR35009
>>>
>>> Reviewers: aaron.ballman, rsmith, rjmccall
>>>
>>> Reviewed By: aaron.ballman
>>>
>>> Subscribers: Rakete, efriedma, materi, cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D39122
>>>
>>> Added:
>>> cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
>>> cfe/trunk/test/Sema/tautological-constant-enum-compare.c
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
>>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: 
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
>>> @@ -8181,8 +8181,12 @@ struct IntRange {
>>>  if (const AtomicType *AT = dyn_cast(T))
>>>T = AT->getValueType().getTypePtr();
>>>
>>> -// For enum types, use the known bit width of the enumerators.
>>> -if (const EnumType *ET = dyn_cast(T)) {
>>> +if (!C.getLangOpts().CPlusPlus) {
>>> +  // For enum types in C code, use the underlying datatype.
>>> +  if (const EnumType *ET = dyn_cast(T))
>>> +T = 
>>> ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
>>> +} else if (const EnumType *ET = dyn_cast(T)) {
>>> +  // For enum types in C++, use the known bit width of the enumerators.
>>>EnumDecl *Enum = ET->getDecl();
>>>// In C++11, enums without definitions can have an explicitly 
>>> specified
>>>// underlying type.  Use this type to compute the range.
>>> @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
>>>  }
>>>
>>>  enum class LimitType {
>>> -  Max, // e.g. 32767 for short
>>> -  Min  // e.g. -32768 for short
>>> +  Max = 1U << 0U,  // e.g. 32767 for short
>>> +  Min = 1U << 1U,  // e.g. -32768 for short
>>> +  Both = Max | Min // When the value is both the Min and the Max limit at 
>>> the
>>> +   // same time; e.g. in C++, A::a in enum A { a = 0 };
>>>  };
>>>
>>>  /// Checks whether Expr 'Constant' may be the
>>> @@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
>>>
>>>IntRange OtherRange = 

Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Roman Lebedev via cfe-commits]

On Mon, Oct 23, 2017 at 2:13 PM, Hans Wennborg  wrote:
Hi.

> This seems to have had the side effect of introducing a new warning in
> Chromium builds:
>
> ../../third_party/expat/files/lib/xmlparse.c(2429,24):  error:
> comparison of integers of different signs: 'enum XML_Error' and
> 'unsigned int' [-Werror,-Wsign-compare]
>   if (code > 0 && code < sizeof(message)/sizeof(message[0]))
>    ^ ~~
(I guess this is on windows)

> I'm not sure if this was intentional or not.
>
> The warning seems technically correct here, though not very useful in
> this specific case.
I *believe* that was caused by the fix for IntRange::forValueOfCanonicalType()
to use enum's underlying type for C code. That fix was absolutely intentional.

However that new -Wsign-compare diagnostic (and i suspect there may be
more repercussions) was not really intentional.
However as you have said, and i think i agree, the diagnostic valid.

So perhaps i simply should add a test and release notes entry?

Roman.

> On Sat, Oct 21, 2017 at 6:44 PM, Roman Lebedev via cfe-commits
>  wrote:
>> Author: lebedevri
>> Date: Sat Oct 21 09:44:03 2017
>> New Revision: 316268
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
>> Log:
>> [Sema] Fixes for enum handling for tautological comparison diagnostics
>>
>> Summary:
>> As Mattias Eriksson has reported in PR35009, in C, for enums, the underlying 
>> type should
>> be used when checking for the tautological comparison, unlike C++, where the 
>> enumerator
>> values define the value range. So if not in CPlusPlus mode, use the enum 
>> underlying type.
>>
>> Also, i have discovered a problem (a crash) when evaluating 
>> tautological-ness of the following comparison:
>> ```
>> enum A { A_a = 0 };
>> if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 
>> is always false}}
>> return 0;
>> ```
>> This affects both the C and C++, but after the first fix, only C++ code was 
>> affected.
>> That was also fixed, while preserving (i think?) the proper diagnostic 
>> output.
>>
>> And while there, attempt to enhance the test coverage.
>> Yes, some tests got moved around, sorry about that :)
>>
>> Fixes PR35009
>>
>> Reviewers: aaron.ballman, rsmith, rjmccall
>>
>> Reviewed By: aaron.ballman
>>
>> Subscribers: Rakete, efriedma, materi, cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D39122
>>
>> Added:
>> cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
>> cfe/trunk/test/Sema/tautological-constant-enum-compare.c
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
>> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
>> @@ -8181,8 +8181,12 @@ struct IntRange {
>>  if (const AtomicType *AT = dyn_cast(T))
>>T = AT->getValueType().getTypePtr();
>>
>> -// For enum types, use the known bit width of the enumerators.
>> -if (const EnumType *ET = dyn_cast(T)) {
>> +if (!C.getLangOpts().CPlusPlus) {
>> +  // For enum types in C code, use the underlying datatype.
>> +  if (const EnumType *ET = dyn_cast(T))
>> +T = 
>> ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
>> +} else if (const EnumType *ET = dyn_cast(T)) {
>> +  // For enum types in C++, use the known bit width of the enumerators.
>>EnumDecl *Enum = ET->getDecl();
>>// In C++11, enums without definitions can have an explicitly 
>> specified
>>// underlying type.  Use this type to compute the range.
>> @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
>>  }
>>
>>  enum class LimitType {
>> -  Max, // e.g. 32767 for short
>> -  Min  // e.g. -32768 for short
>> +  Max = 1U << 0U,  // e.g. 32767 for short
>> +  Min = 1U << 1U,  // e.g. -32768 for short
>> +  Both = Max | Min // When the value is both the Min and the Max limit at 
>> the
>> +   // same time; e.g. in C++, A::a in enum A { a = 0 };
>>  };
>>
>>  /// Checks whether Expr 'Constant' may be the
>> @@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
>>
>>IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>>
>> +  // Special-case for C++ for enum with one enumerator with value of 0.
>> +  if (OtherRange.Width == 0)
>> +return Value == 0 ? LimitType::Both : llvm::Optional();
>> +
>>if (llvm::APSInt::isSameValue(
>>llvm::APSInt::getMaxValue(OtherRange.Width,
>>  

Re: r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Hans Wennborg via cfe-commits]

This seems to have had the side effect of introducing a new warning in
Chromium builds:

../../third_party/expat/files/lib/xmlparse.c(2429,24):  error:
comparison of integers of different signs: 'enum XML_Error' and
'unsigned int' [-Werror,-Wsign-compare]
  if (code > 0 && code < sizeof(message)/sizeof(message[0]))
   ^ ~~

I'm not sure if this was intentional or not.

The warning seems technically correct here, though not very useful in
this specific case.

On Sat, Oct 21, 2017 at 6:44 PM, Roman Lebedev via cfe-commits
 wrote:
> Author: lebedevri
> Date: Sat Oct 21 09:44:03 2017
> New Revision: 316268
>
> URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
> Log:
> [Sema] Fixes for enum handling for tautological comparison diagnostics
>
> Summary:
> As Mattias Eriksson has reported in PR35009, in C, for enums, the underlying 
> type should
> be used when checking for the tautological comparison, unlike C++, where the 
> enumerator
> values define the value range. So if not in CPlusPlus mode, use the enum 
> underlying type.
>
> Also, i have discovered a problem (a crash) when evaluating tautological-ness 
> of the following comparison:
> ```
> enum A { A_a = 0 };
> if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 
> is always false}}
> return 0;
> ```
> This affects both the C and C++, but after the first fix, only C++ code was 
> affected.
> That was also fixed, while preserving (i think?) the proper diagnostic output.
>
> And while there, attempt to enhance the test coverage.
> Yes, some tests got moved around, sorry about that :)
>
> Fixes PR35009
>
> Reviewers: aaron.ballman, rsmith, rjmccall
>
> Reviewed By: aaron.ballman
>
> Subscribers: Rakete, efriedma, materi, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D39122
>
> Added:
> cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
> cfe/trunk/test/Sema/tautological-constant-enum-compare.c
> Modified:
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
> cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
> @@ -8181,8 +8181,12 @@ struct IntRange {
>  if (const AtomicType *AT = dyn_cast(T))
>T = AT->getValueType().getTypePtr();
>
> -// For enum types, use the known bit width of the enumerators.
> -if (const EnumType *ET = dyn_cast(T)) {
> +if (!C.getLangOpts().CPlusPlus) {
> +  // For enum types in C code, use the underlying datatype.
> +  if (const EnumType *ET = dyn_cast(T))
> +T = ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
> +} else if (const EnumType *ET = dyn_cast(T)) {
> +  // For enum types in C++, use the known bit width of the enumerators.
>EnumDecl *Enum = ET->getDecl();
>// In C++11, enums without definitions can have an explicitly specified
>// underlying type.  Use this type to compute the range.
> @@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
>  }
>
>  enum class LimitType {
> -  Max, // e.g. 32767 for short
> -  Min  // e.g. -32768 for short
> +  Max = 1U << 0U,  // e.g. 32767 for short
> +  Min = 1U << 1U,  // e.g. -32768 for short
> +  Both = Max | Min // When the value is both the Min and the Max limit at the
> +   // same time; e.g. in C++, A::a in enum A { a = 0 };
>  };
>
>  /// Checks whether Expr 'Constant' may be the
> @@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
>
>IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>
> +  // Special-case for C++ for enum with one enumerator with value of 0.
> +  if (OtherRange.Width == 0)
> +return Value == 0 ? LimitType::Both : llvm::Optional();
> +
>if (llvm::APSInt::isSameValue(
>llvm::APSInt::getMaxValue(OtherRange.Width,
>  OtherT->isUnsignedIntegerType()),
> @@ -8620,7 +8630,7 @@ llvm::Optional IsTypeLimit(Se
>Value))
>  return LimitType::Min;
>
> -  return llvm::Optional();
> +  return llvm::None;
>  }
>
>  bool HasEnumType(Expr *E) {
> @@ -8655,9 +8665,12 @@ bool CheckTautologicalComparison(Sema 
>
>bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
>bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
> -  bool ResultWhenConstNeOther =
> -  ConstIsLowerBound ^ (ValueType == LimitType::Max);
> -  if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
> +  if (ValueType != LimitType::Both) {
> +bool ResultWhenConstNeOther =
> +   

r316268 - [Sema] Fixes for enum handling for tautological comparison diagnostics

[Roman Lebedev via cfe-commits]

Author: lebedevri
Date: Sat Oct 21 09:44:03 2017
New Revision: 316268

URL: http://llvm.org/viewvc/llvm-project?rev=316268=rev
Log:
[Sema] Fixes for enum handling for tautological comparison diagnostics

Summary:
As Mattias Eriksson has reported in PR35009, in C, for enums, the underlying 
type should
be used when checking for the tautological comparison, unlike C++, where the 
enumerator
values define the value range. So if not in CPlusPlus mode, use the enum 
underlying type.

Also, i have discovered a problem (a crash) when evaluating tautological-ness 
of the following comparison:
```
enum A { A_a = 0 };
if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 is 
always false}}
return 0;
```
This affects both the C and C++, but after the first fix, only C++ code was 
affected.
That was also fixed, while preserving (i think?) the proper diagnostic output.

And while there, attempt to enhance the test coverage.
Yes, some tests got moved around, sorry about that :)

Fixes PR35009

Reviewers: aaron.ballman, rsmith, rjmccall

Reviewed By: aaron.ballman

Subscribers: Rakete, efriedma, materi, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D39122

Added:
cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
cfe/trunk/test/Sema/tautological-constant-enum-compare.c
Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.c
cfe/trunk/test/Sema/tautological-unsigned-enum-zero-compare.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=316268=316267=316268=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Sat Oct 21 09:44:03 2017
@@ -8181,8 +8181,12 @@ struct IntRange {
 if (const AtomicType *AT = dyn_cast(T))
   T = AT->getValueType().getTypePtr();
 
-// For enum types, use the known bit width of the enumerators.
-if (const EnumType *ET = dyn_cast(T)) {
+if (!C.getLangOpts().CPlusPlus) {
+  // For enum types in C code, use the underlying datatype.
+  if (const EnumType *ET = dyn_cast(T))
+T = ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
+} else if (const EnumType *ET = dyn_cast(T)) {
+  // For enum types in C++, use the known bit width of the enumerators.
   EnumDecl *Enum = ET->getDecl();
   // In C++11, enums without definitions can have an explicitly specified
   // underlying type.  Use this type to compute the range.
@@ -8584,8 +8588,10 @@ bool isNonBooleanUnsignedValue(Expr *E)
 }
 
 enum class LimitType {
-  Max, // e.g. 32767 for short
-  Min  // e.g. -32768 for short
+  Max = 1U << 0U,  // e.g. 32767 for short
+  Min = 1U << 1U,  // e.g. -32768 for short
+  Both = Max | Min // When the value is both the Min and the Max limit at the
+   // same time; e.g. in C++, A::a in enum A { a = 0 };
 };
 
 /// Checks whether Expr 'Constant' may be the
@@ -8608,6 +8614,10 @@ llvm::Optional IsTypeLimit(Se
 
   IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
 
+  // Special-case for C++ for enum with one enumerator with value of 0.
+  if (OtherRange.Width == 0)
+return Value == 0 ? LimitType::Both : llvm::Optional();
+
   if (llvm::APSInt::isSameValue(
   llvm::APSInt::getMaxValue(OtherRange.Width,
 OtherT->isUnsignedIntegerType()),
@@ -8620,7 +8630,7 @@ llvm::Optional IsTypeLimit(Se
   Value))
 return LimitType::Min;
 
-  return llvm::Optional();
+  return llvm::None;
 }
 
 bool HasEnumType(Expr *E) {
@@ -8655,9 +8665,12 @@ bool CheckTautologicalComparison(Sema 
 
   bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
   bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
-  bool ResultWhenConstNeOther =
-  ConstIsLowerBound ^ (ValueType == LimitType::Max);
-  if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
+  if (ValueType != LimitType::Both) {
+bool ResultWhenConstNeOther =
+ConstIsLowerBound ^ (ValueType == LimitType::Max);
+if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
+  return false; // The comparison is not tautological.
+  } else if (ResultWhenConstEqualsOther == ConstIsLowerBound)
 return false; // The comparison is not tautological.
 
   const bool Result = ResultWhenConstEqualsOther;

Added: cfe/trunk/test/Sema/outof-range-enum-constant-compare.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/outof-range-enum-constant-compare.c?rev=316268=auto
==
--- cfe/trunk/test/Sema/outof-range-enum-constant-compare.c (added)
+++ cfe/trunk/test/Sema/outof-range-enum-constant-compare.c Sat Oct 21 09:44:03 
2017
@@ -0,0 +1,379 @@
+// RUN: %clang_cc1