r320133 - Revert r320124 "Fold together the in-range and out-of-range portions of -Wtautological-compare."
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 7 21:19:13 PST 2017
Author: hans
Date: Thu Dec 7 21:19:12 2017
New Revision: 320133
URL: http://llvm.org/viewvc/llvm-project?rev=320133&view=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&r1=320132&r2=320133&view=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 &Value,
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 &S, BinaryOperator *E,
+ Expr *Constant, Expr *Other,
+ const llvm::APSInt &Value,
+ 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<AtomicType>())
+ 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, this comparison is not
+ // tautological.
+ if (Cmp & PromotedRange::InRangeFlag)
+ return false;
+
+ auto IsTrue = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant);
+ if (!IsTrue)
return false;
// If this is a comparison to an enum constant, include that
@@ -8853,7 +8929,6 @@ static bool CheckTautologicalComparison(
if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant))
ED = dyn_cast<EnumConstantDecl>(DR->getDecl());
- // Should be enough for uint128 (39 decimal digits)
SmallString<64> PrettySourceValue;
llvm::raw_svector_ostream OS(PrettySourceValue);
if (ED)
@@ -8861,30 +8936,14 @@ static bool CheckTautologicalComparison(
else
OS << Value;
- // FIXME: We use a somewhat different formatting for the in-range cases and
- // cases involving boolean values for historical reasons. We should pick a
- // consistent way of presenting these diagnostics.
- if (!InRange || Other->isKnownToHaveBooleanValue()) {
- S.DiagRuntimeBehavior(
- E->getOperatorLoc(), E,
- S.PDiag(!InRange ? diag::warn_out_of_range_compare
- : diag::warn_tautological_bool_compare)
- << OS.str() << classifyConstantValue(Constant)
- << OtherT << OtherIsBooleanDespiteType << *Result
- << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
- } else {
- 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();
- }
+ S.DiagRuntimeBehavior(
+ E->getOperatorLoc(), E,
+ S.PDiag(diag::warn_out_of_range_compare)
+ << OS.str() << classifyConstantValue(Constant)
+ << OtherT << OtherIsBooleanDespiteType << *IsTrue
+ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
- return true;
+ return true;
}
/// Analyze the operands of the given comparison. Implements the
@@ -8934,8 +8993,12 @@ static void AnalyzeComparison(Sema &S, B
// Check whether an integer constant comparison results in a value
// of 'true' or 'false'.
+
if (CheckTautologicalComparison(S, E, Const, Other, Value, RhsConstant))
return AnalyzeImpConvsInComparison(S, E);
+
+ if (DiagnoseOutOfRangeComparison(S, E, Const, Other, Value, RhsConstant))
+ return AnalyzeImpConvsInComparison(S, E);
}
}
More information about the cfe-commits
mailing list