Re: r320124 - Fold together the in-range and out-of-range portions of -Wtautological-compare.

[Hans Wennborg via cfe-commits]

Sorry, it seems it was r320124 that caused this; I shouldn't be
sheriffing after-hours.

I've reverted that one in r320162.

On Thu, Dec 7, 2017 at 9:20 PM, Hans Wennborg  wrote:
> I've reverted in r320133 since it caused new warnings in Chromium that
> I'm not sure were intentional. See comment on the revert.
>
> On Thu, Dec 7, 2017 at 5:00 PM, Richard Smith via cfe-commits
>  wrote:
>> Author: rsmith
>> Date: Thu Dec  7 17:00:27 2017
>> New Revision: 320124
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=320124=rev
>> Log:
>> Fold together the in-range and out-of-range portions of 
>> -Wtautological-compare.
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320124=320123=320124=diff
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec  7 17:00:27 2017
>> @@ -8801,12 +8801,7 @@ static bool CheckTautologicalComparison(
>>  Expr *Constant, Expr *Other,
>>  const llvm::APSInt ,
>>  bool RhsConstant) {
>> -  // Disable warning in template instantiations
>> -  // and only analyze <, >, <= and >= operations.
>> -  if (S.inTemplateInstantiation() || !E->isRelationalOp())
>> -return false;
>> -
>> -  if (IsEnumConstOrFromMacro(S, Constant))
>> +  if (S.inTemplateInstantiation())
>>  return false;
>>
>>Expr *OriginalOther = Other;
>> @@ -8833,94 +8828,23 @@ static bool CheckTautologicalComparison(
>>OtherRange.Width =
>>std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
>>
>> -  // Check whether the constant value can be represented in OtherRange. Bail
>> -  // out if so; this isn't an out-of-range comparison.
>> +  // Determine the promoted range of the other type and see if a comparison 
>> of
>> +  // the constant against that range is tautological.
>>PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
>> Value.isUnsigned());
>> -
>>auto Cmp = OtherPromotedRange.compare(Value);
>> -  if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max &&
>> -  Cmp != PromotedRange::OnlyValue)
>> -return false;
>> -
>>auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, 
>> RhsConstant);
>>if (!Result)
>>  return false;
>>
>> -  // Should be enough for uint128 (39 decimal digits)
>> -  SmallString<64> PrettySourceValue;
>> -  llvm::raw_svector_ostream OS(PrettySourceValue);
>> -  OS << Value;
>> -
>> -  // FIXME: We use a somewhat different formatting for the cases involving
>> -  // boolean values for historical reasons. We should pick a consistent way
>> -  // of presenting these diagnostics.
>> -  if (Other->isKnownToHaveBooleanValue()) {
>> -S.DiagRuntimeBehavior(
>> -  E->getOperatorLoc(), E,
>> -  S.PDiag(diag::warn_tautological_bool_compare)
>> -  << OS.str() << classifyConstantValue(Constant)
>> -  << OtherT << !OtherT->isBooleanType() << *Result
>> -  << E->getLHS()->getSourceRange() << 
>> E->getRHS()->getSourceRange());
>> -return true;
>> -  }
>> -
>> -  unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0)
>> -  ? (HasEnumType(OriginalOther)
>> - ? 
>> diag::warn_unsigned_enum_always_true_comparison
>> - : diag::warn_unsigned_always_true_comparison)
>> -  : diag::warn_tautological_constant_compare;
>> -
>> -  S.Diag(E->getOperatorLoc(), Diag)
>> -  << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result
>> -  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
>> -
>> -  return true;
>> -}
>> -
>> -static bool DiagnoseOutOfRangeComparison(Sema , BinaryOperator *E,
>> - Expr *Constant, Expr *Other,
>> - const llvm::APSInt ,
>> - bool RhsConstant) {
>> -  // Disable warning in template instantiations.
>> -  if (S.inTemplateInstantiation())
>> -return false;
>> -
>> -  Constant = Constant->IgnoreParenImpCasts();
>> -  Other = Other->IgnoreParenImpCasts();
>> -
>> -  // TODO: Investigate using GetExprRange() to get tighter bounds
>> -  // on the bit ranges.
>> -  QualType OtherT = Other->getType();
>> -  if (const auto *AT = OtherT->getAs())
>> -OtherT = AT->getValueType();
>> -  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>> -
>> -  // Whether we're treating Other as being a bool because of the form of
>> -  // expression despite it having another type (typically 'int' in C).
>> -  bool 

Re: r320124 - Fold together the in-range and out-of-range portions of -Wtautological-compare.

[Hans Wennborg via cfe-commits]

I've reverted in r320133 since it caused new warnings in Chromium that
I'm not sure were intentional. See comment on the revert.

On Thu, Dec 7, 2017 at 5:00 PM, Richard Smith via cfe-commits
 wrote:
> Author: rsmith
> Date: Thu Dec  7 17:00:27 2017
> New Revision: 320124
>
> URL: http://llvm.org/viewvc/llvm-project?rev=320124=rev
> Log:
> Fold together the in-range and out-of-range portions of 
> -Wtautological-compare.
>
> Modified:
> cfe/trunk/lib/Sema/SemaChecking.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320124=320123=320124=diff
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec  7 17:00:27 2017
> @@ -8801,12 +8801,7 @@ static bool CheckTautologicalComparison(
>  Expr *Constant, Expr *Other,
>  const llvm::APSInt ,
>  bool RhsConstant) {
> -  // Disable warning in template instantiations
> -  // and only analyze <, >, <= and >= operations.
> -  if (S.inTemplateInstantiation() || !E->isRelationalOp())
> -return false;
> -
> -  if (IsEnumConstOrFromMacro(S, Constant))
> +  if (S.inTemplateInstantiation())
>  return false;
>
>Expr *OriginalOther = Other;
> @@ -8833,94 +8828,23 @@ static bool CheckTautologicalComparison(
>OtherRange.Width =
>std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
>
> -  // Check whether the constant value can be represented in OtherRange. Bail
> -  // out if so; this isn't an out-of-range comparison.
> +  // Determine the promoted range of the other type and see if a comparison 
> of
> +  // the constant against that range is tautological.
>PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
> Value.isUnsigned());
> -
>auto Cmp = OtherPromotedRange.compare(Value);
> -  if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max &&
> -  Cmp != PromotedRange::OnlyValue)
> -return false;
> -
>auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, 
> RhsConstant);
>if (!Result)
>  return false;
>
> -  // Should be enough for uint128 (39 decimal digits)
> -  SmallString<64> PrettySourceValue;
> -  llvm::raw_svector_ostream OS(PrettySourceValue);
> -  OS << Value;
> -
> -  // FIXME: We use a somewhat different formatting for the cases involving
> -  // boolean values for historical reasons. We should pick a consistent way
> -  // of presenting these diagnostics.
> -  if (Other->isKnownToHaveBooleanValue()) {
> -S.DiagRuntimeBehavior(
> -  E->getOperatorLoc(), E,
> -  S.PDiag(diag::warn_tautological_bool_compare)
> -  << OS.str() << classifyConstantValue(Constant)
> -  << OtherT << !OtherT->isBooleanType() << *Result
> -  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
> -return true;
> -  }
> -
> -  unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0)
> -  ? (HasEnumType(OriginalOther)
> - ? 
> diag::warn_unsigned_enum_always_true_comparison
> - : diag::warn_unsigned_always_true_comparison)
> -  : diag::warn_tautological_constant_compare;
> -
> -  S.Diag(E->getOperatorLoc(), Diag)
> -  << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result
> -  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
> -
> -  return true;
> -}
> -
> -static bool DiagnoseOutOfRangeComparison(Sema , BinaryOperator *E,
> - Expr *Constant, Expr *Other,
> - const llvm::APSInt ,
> - bool RhsConstant) {
> -  // Disable warning in template instantiations.
> -  if (S.inTemplateInstantiation())
> -return false;
> -
> -  Constant = Constant->IgnoreParenImpCasts();
> -  Other = Other->IgnoreParenImpCasts();
> -
> -  // TODO: Investigate using GetExprRange() to get tighter bounds
> -  // on the bit ranges.
> -  QualType OtherT = Other->getType();
> -  if (const auto *AT = OtherT->getAs())
> -OtherT = AT->getValueType();
> -  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
> -
> -  // Whether we're treating Other as being a bool because of the form of
> -  // expression despite it having another type (typically 'int' in C).
> -  bool OtherIsBooleanDespiteType =
> -  !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue();
> -  if (OtherIsBooleanDespiteType)
> -OtherRange = IntRange::forBoolType();
> -
> -  if (FieldDecl *Bitfield = Other->getSourceBitField())
> -if (!Bitfield->getBitWidth()->isValueDependent())
> -  OtherRange.Width 

r320133 - Revert r320124 "Fold together the in-range and out-of-range portions of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

Author: hans
Date: Thu Dec  7 21:19:12 2017
New Revision: 320133

URL: http://llvm.org/viewvc/llvm-project?rev=320133=rev
Log:
Revert r320124 "Fold together the in-range and out-of-range portions of 
-Wtautological-compare."

This broke Chromium:

../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant 64
with expression of type 'unsigned int' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
  DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
 ~~ ^ ~~~

The 'unsigned int' is really a 6-bit bitfield, which is why it's always
less than 63.

Did this use to fall under the "in-range" case before? I thought we
didn't use to warn when comparing against the boundaries of a type.

Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320133=320132=320133=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec  7 21:19:12 2017
@@ -8801,7 +8801,12 @@ static bool CheckTautologicalComparison(
 Expr *Constant, Expr *Other,
 const llvm::APSInt ,
 bool RhsConstant) {
-  if (S.inTemplateInstantiation())
+  // Disable warning in template instantiations
+  // and only analyze <, >, <= and >= operations.
+  if (S.inTemplateInstantiation() || !E->isRelationalOp())
+return false;
+
+  if (IsEnumConstOrFromMacro(S, Constant))
 return false;
 
   Expr *OriginalOther = Other;
@@ -8828,23 +8833,94 @@ static bool CheckTautologicalComparison(
   OtherRange.Width =
   std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
 
-  // Determine the promoted range of the other type and see if a comparison of
-  // the constant against that range is tautological.
+  // Check whether the constant value can be represented in OtherRange. Bail
+  // out if so; this isn't an out-of-range comparison.
   PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
Value.isUnsigned());
+
   auto Cmp = OtherPromotedRange.compare(Value);
+  if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max &&
+  Cmp != PromotedRange::OnlyValue)
+return false;
+
   auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant);
   if (!Result)
 return false;
 
-  // Suppress the diagnostic for an in-range comparison if the constant comes
-  // from a macro or enumerator. We don't want to diagnose
-  //
-  //   some_long_value <= INT_MAX
-  //
-  // when sizeof(int) == sizeof(long).
-  bool InRange = Cmp & PromotedRange::InRangeFlag;
-  if (InRange && IsEnumConstOrFromMacro(S, Constant))
+  // Should be enough for uint128 (39 decimal digits)
+  SmallString<64> PrettySourceValue;
+  llvm::raw_svector_ostream OS(PrettySourceValue);
+  OS << Value;
+
+  // FIXME: We use a somewhat different formatting for the cases involving
+  // boolean values for historical reasons. We should pick a consistent way
+  // of presenting these diagnostics.
+  if (Other->isKnownToHaveBooleanValue()) {
+S.DiagRuntimeBehavior(
+  E->getOperatorLoc(), E,
+  S.PDiag(diag::warn_tautological_bool_compare)
+  << OS.str() << classifyConstantValue(Constant)
+  << OtherT << !OtherT->isBooleanType() << *Result
+  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
+return true;
+  }
+
+  unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0)
+  ? (HasEnumType(OriginalOther)
+ ? diag::warn_unsigned_enum_always_true_comparison
+ : diag::warn_unsigned_always_true_comparison)
+  : diag::warn_tautological_constant_compare;
+
+  S.Diag(E->getOperatorLoc(), Diag)
+  << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result
+  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+
+  return true;
+}
+
+static bool DiagnoseOutOfRangeComparison(Sema , BinaryOperator *E,
+ Expr *Constant, Expr *Other,
+ const llvm::APSInt ,
+ bool RhsConstant) {
+  // Disable warning in template instantiations.
+  if (S.inTemplateInstantiation())
+return false;
+
+  Constant = Constant->IgnoreParenImpCasts();
+  Other = Other->IgnoreParenImpCasts();
+
+  // TODO: Investigate using GetExprRange() to get tighter bounds
+  // on the bit ranges.
+  QualType OtherT = Other->getType();
+  if (const auto *AT = OtherT->getAs())
+OtherT = AT->getValueType();
+  IntRange OtherRange = IntRange::forValueOfType(S.Context, 

r320124 - Fold together the in-range and out-of-range portions of -Wtautological-compare.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Thu Dec  7 17:00:27 2017
New Revision: 320124

URL: http://llvm.org/viewvc/llvm-project?rev=320124=rev
Log:
Fold together the in-range and out-of-range portions of -Wtautological-compare.

Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320124=320123=320124=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec  7 17:00:27 2017
@@ -8801,12 +8801,7 @@ static bool CheckTautologicalComparison(
 Expr *Constant, Expr *Other,
 const llvm::APSInt ,
 bool RhsConstant) {
-  // Disable warning in template instantiations
-  // and only analyze <, >, <= and >= operations.
-  if (S.inTemplateInstantiation() || !E->isRelationalOp())
-return false;
-
-  if (IsEnumConstOrFromMacro(S, Constant))
+  if (S.inTemplateInstantiation())
 return false;
 
   Expr *OriginalOther = Other;
@@ -8833,94 +8828,23 @@ static bool CheckTautologicalComparison(
   OtherRange.Width =
   std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
 
-  // Check whether the constant value can be represented in OtherRange. Bail
-  // out if so; this isn't an out-of-range comparison.
+  // Determine the promoted range of the other type and see if a comparison of
+  // the constant against that range is tautological.
   PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
Value.isUnsigned());
-
   auto Cmp = OtherPromotedRange.compare(Value);
-  if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max &&
-  Cmp != PromotedRange::OnlyValue)
-return false;
-
   auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant);
   if (!Result)
 return false;
 
-  // Should be enough for uint128 (39 decimal digits)
-  SmallString<64> PrettySourceValue;
-  llvm::raw_svector_ostream OS(PrettySourceValue);
-  OS << Value;
-
-  // FIXME: We use a somewhat different formatting for the cases involving
-  // boolean values for historical reasons. We should pick a consistent way
-  // of presenting these diagnostics.
-  if (Other->isKnownToHaveBooleanValue()) {
-S.DiagRuntimeBehavior(
-  E->getOperatorLoc(), E,
-  S.PDiag(diag::warn_tautological_bool_compare)
-  << OS.str() << classifyConstantValue(Constant)
-  << OtherT << !OtherT->isBooleanType() << *Result
-  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
-return true;
-  }
-
-  unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0)
-  ? (HasEnumType(OriginalOther)
- ? diag::warn_unsigned_enum_always_true_comparison
- : diag::warn_unsigned_always_true_comparison)
-  : diag::warn_tautological_constant_compare;
-
-  S.Diag(E->getOperatorLoc(), Diag)
-  << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result
-  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-
-  return true;
-}
-
-static bool DiagnoseOutOfRangeComparison(Sema , BinaryOperator *E,
- Expr *Constant, Expr *Other,
- const llvm::APSInt ,
- bool RhsConstant) {
-  // Disable warning in template instantiations.
-  if (S.inTemplateInstantiation())
-return false;
-
-  Constant = Constant->IgnoreParenImpCasts();
-  Other = Other->IgnoreParenImpCasts();
-
-  // TODO: Investigate using GetExprRange() to get tighter bounds
-  // on the bit ranges.
-  QualType OtherT = Other->getType();
-  if (const auto *AT = OtherT->getAs())
-OtherT = AT->getValueType();
-  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
-
-  // Whether we're treating Other as being a bool because of the form of
-  // expression despite it having another type (typically 'int' in C).
-  bool OtherIsBooleanDespiteType =
-  !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue();
-  if (OtherIsBooleanDespiteType)
-OtherRange = IntRange::forBoolType();
-
-  if (FieldDecl *Bitfield = Other->getSourceBitField())
-if (!Bitfield->getBitWidth()->isValueDependent())
-  OtherRange.Width =
-  std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
-
-  // Check whether the constant value can be represented in OtherRange. Bail
-  // out if so; this isn't an out-of-range comparison.
-  PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
-   Value.isUnsigned());
-  auto Cmp = OtherPromotedRange.compare(Value);
-
-  // If Value is in the range of possible Other values,