lebedev.ri created this revision.
lebedev.ri added a project: clang.
Currently, clang only diagnoses completely out-of-range comparisons (e.g.
`char` and constant `300`),
and comparisons of unsigned and `0`. But gcc also does diagnose the comparisons
with the
`std::numeric_limits<>::max()` / `std::numeric_limits<>::min()` so to speak
I don't really like this code, but given the previous feedback in
https://reviews.llvm.org/D37629,
i decided to do it in the most simple way possible for now, and change it given
on review feedback.
Finally Fixes https://bugs.llvm.org/show_bug.cgi?id=34147
Continuation of https://reviews.llvm.org/D37565
Repository:
rL LLVM
https://reviews.llvm.org/D38101
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/Sema/outof-range-constant-compare.c
Index: test/Sema/outof-range-constant-compare.c
===================================================================
--- test/Sema/outof-range-constant-compare.c
+++ test/Sema/outof-range-constant-compare.c
@@ -102,6 +102,7 @@
return 1;
short s = value();
+
if (s == 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always false}}
return 0;
if (s != 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
@@ -128,6 +129,112 @@
if (0x1234567812345678L >= s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
return 0;
+ if (s == 32767)
+ return 0;
+ if (s != 32767)
+ return 0;
+ if (s < 32767)
+ return 0;
+ if (s <= 32767) // expected-warning {{comparison 'short' <= 32767 is always true}}
+ return 0;
+ if (s > 32767) // expected-warning {{comparison 'short' > 32767 is always false}}
+ return 0;
+ if (s >= 32767)
+ return 0;
+
+ if (32767 == s)
+ return 0;
+ if (32767 != s)
+ return 0;
+ if (32767 < s) // expected-warning {{comparison 32767 < 'short' is always false}}
+ return 0;
+ if (32767 <= s)
+ return 0;
+ if (32767 > s)
+ return 0;
+ if (32767 >= s) // expected-warning {{comparison 32767 >= 'short' is always true}}
+ return 0;
+
+ // FIXME: assumes two's complement
+ if (s == -32768)
+ return 0;
+ if (s != -32768)
+ return 0;
+ if (s < -32768) // expected-warning {{comparison 'short' < -32768 is always false}}
+ return 0;
+ if (s <= -32768)
+ return 0;
+ if (s > -32768)
+ return 0;
+ if (s >= -32768) // expected-warning {{comparison 'short' >= -32768 is always true}}
+ return 0;
+
+ if (-32768 == s)
+ return 0;
+ if (-32768 != s)
+ return 0;
+ if (-32768 < s)
+ return 0;
+ if (-32768 <= s) // expected-warning {{comparison -32768 <= 'short' is always true}}
+ return 0;
+ if (-32768 > s) // expected-warning {{comparison -32768 > 'short' is always false}}
+ return 0;
+ if (-32768 >= s)
+ return 0;
+
+ if (s == 32767UL)
+ return 0;
+ if (s != 32767UL)
+ return 0;
+ if (s < 32767UL)
+ return 0;
+ if (s <= 32767UL) // expected-warning {{comparison 'short' <= 32767 is always true}}
+ return 0;
+ if (s > 32767UL) // expected-warning {{comparison 'short' > 32767 is always false}}
+ return 0;
+ if (s >= 32767UL)
+ return 0;
+
+ if (32767UL == s)
+ return 0;
+ if (32767UL != s)
+ return 0;
+ if (32767UL < s) // expected-warning {{comparison 32767 < 'short' is always false}}
+ return 0;
+ if (32767UL <= s)
+ return 0;
+ if (32767UL > s)
+ return 0;
+ if (32767UL >= s) // expected-warning {{comparison 32767 >= 'short' is always true}}
+ return 0;
+
+ // FIXME: assumes two's complement
+ if (s == -32768L)
+ return 0;
+ if (s != -32768L)
+ return 0;
+ if (s < -32768L) // expected-warning {{comparison 'short' < -32768 is always false}}
+ return 0;
+ if (s <= -32768L)
+ return 0;
+ if (s > -32768L)
+ return 0;
+ if (s >= -32768L) // expected-warning {{comparison 'short' >= -32768 is always true}}
+ return 0;
+
+ if (-32768L == s)
+ return 0;
+ if (-32768L != s)
+ return 0;
+ if (-32768L < s)
+ return 0;
+ if (-32768L <= s) // expected-warning {{comparison -32768 <= 'short' is always true}}
+ return 0;
+ if (-32768L > s) // expected-warning {{comparison -32768 > 'short' is always false}}
+ return 0;
+ if (-32768L >= s)
+ return 0;
+
long l = value();
if (l == 0x1234567812345678L)
return 0;
@@ -208,6 +315,60 @@
if (0x0000000000000000UL >= un)
return 0;
+ unsigned short us = value();
+
+ if (us == 65535)
+ return 0;
+ if (us != 65535)
+ return 0;
+ if (us < 65535)
+ return 0;
+ if (us <= 65535) // expected-warning {{comparison 'unsigned short' <= 65535 is always true}}
+ return 0;
+ if (us > 65535) // expected-warning {{comparison 'unsigned short' > 65535 is always false}}
+ return 0;
+ if (us >= 65535)
+ return 0;
+
+ if (65535 == us)
+ return 0;
+ if (65535 != us)
+ return 0;
+ if (65535 < us) // expected-warning {{comparison 65535 < 'unsigned short' is always false}}
+ return 0;
+ if (65535 <= us)
+ return 0;
+ if (65535 > us)
+ return 0;
+ if (65535 >= us) // expected-warning {{comparison 65535 >= 'unsigned short' is always true}}
+ return 0;
+
+ if (us == 65535UL)
+ return 0;
+ if (us != 65535UL)
+ return 0;
+ if (us < 65535UL)
+ return 0;
+ if (us <= 65535UL) // expected-warning {{comparison 'unsigned short' <= 65535 is always true}}
+ return 0;
+ if (us > 65535UL) // expected-warning {{comparison 'unsigned short' > 65535 is always false}}
+ return 0;
+ if (us >= 65535UL)
+ return 0;
+
+ if (65535UL == us)
+ return 0;
+ if (65535UL != us)
+ return 0;
+ if (65535UL < us) // expected-warning {{comparison 65535 < 'unsigned short' is always false}}
+ return 0;
+ if (65535UL <= us)
+ return 0;
+ if (65535UL > us)
+ return 0;
+ if (65535UL >= us) // expected-warning {{comparison 65535 >= 'unsigned short' is always true}}
+ return 0;
+
float fl = 0;
if (fl == 0x0000000000000000L)
return 0;
@@ -272,5 +433,57 @@
if (e == 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'enum E' is always false}}
return 0;
+ if (e == yes)
+ return 0;
+ if (e != yes)
+ return 0;
+ if (e < yes)
+ return 0;
+ if (e <= yes)
+ return 0;
+ if (e > yes)
+ return 0;
+ if (e >= yes)
+ return 0;
+
+ if (yes == e)
+ return 0;
+ if (yes != e)
+ return 0;
+ if (yes < e)
+ return 0;
+ if (yes <= e)
+ return 0;
+ if (yes > e)
+ return 0;
+ if (yes >= e)
+ return 0;
+
+ if (e == maybe)
+ return 0;
+ if (e != maybe)
+ return 0;
+ if (e < maybe)
+ return 0;
+ if (e <= maybe)
+ return 0;
+ if (e > maybe)
+ return 0;
+ if (e >= maybe)
+ return 0;
+
+ if (maybe == e)
+ return 0;
+ if (maybe != e)
+ return 0;
+ if (maybe < e)
+ return 0;
+ if (maybe <= e)
+ return 0;
+ if (maybe > e)
+ return 0;
+ if (maybe >= e)
+ return 0;
+
return 1;
}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8540,21 +8540,69 @@
void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC);
-bool IsZero(Sema &S, Expr *E) {
+bool IsEnumConstOrFromMacro(Sema &S, Expr *E) {
// Suppress cases where we are comparing against an enum constant.
if (const DeclRefExpr *DR =
dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
if (isa<EnumConstantDecl>(DR->getDecl()))
- return false;
+ return true;
// Suppress cases where the '0' value is expanded from a macro.
if (E->getLocStart().isMacroID())
+ return true;
+
+ return false;
+}
+
+bool IsZero(Sema &S, Expr *E) {
+ if(IsEnumConstOrFromMacro(S, E))
return false;
llvm::APSInt Value;
return E->isIntegerConstantExpr(Value, S.Context) && Value == 0;
}
+enum class LimitType {
+ Max, // e.g. 32767 for short
+ Min // e.g. -32768 for short
+};
+
+// Checks whether Expr 'Constant' may be the
+// std::numeric_limits<>::max() or std::numeric_limits<>::min()
+// of the Expr 'Other'. If true, then returns the limit type (min or max).
+// Does not consider 0 to be type limit. IsZero() and friends do that already.
+llvm::Optional<LimitType> IsTypeLimit(Sema &S, Expr *Other, Expr *Constant,
+ llvm::APSInt *Value) {
+ if(IsEnumConstOrFromMacro(S, Constant))
+ return llvm::Optional<LimitType>();
+
+ // Skip cases where this 'constant' is not an integer constant.
+ if (!Constant->isIntegerConstantExpr(*Value, S.Context))
+ return llvm::Optional<LimitType>();
+
+ // Skip the cases where constant is '0'.
+ if (IsZero(S, Constant))
+ return llvm::Optional<LimitType>();
+
+ // TODO: Investigate using GetExprRange() to get tighter bounds
+ // on the bit ranges.
+ QualType OtherT = Other->IgnoreParenImpCasts()->getType();
+ if (const auto *AT = OtherT->getAs<AtomicType>())
+ OtherT = AT->getValueType();
+ IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
+ unsigned OtherWidth = OtherRange.Width;
+
+ if (llvm::APSInt::getMaxValue(OtherWidth, OtherT->isUnsignedIntegerType())
+ .getExtValue() == Value->getExtValue())
+ return LimitType::Max;
+
+ if (llvm::APSInt::getMinValue(OtherWidth, OtherT->isUnsignedIntegerType())
+ .getExtValue() == Value->getExtValue())
+ return LimitType::Min;
+
+ return llvm::Optional<LimitType>();
+}
+
bool HasEnumType(Expr *E) {
// Strip off implicit integral promotions.
while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
@@ -8567,11 +8615,17 @@
return E->getType()->isEnumeralType();
}
+bool isNonBooleanIntegerValue(Expr *E) {
+ // We are checking that the expression is not known to have boolean value,
+ // is an integer type
+ return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType();
+}
+
bool isNonBooleanUnsignedValue(Expr *E) {
// We are checking that the expression is not known to have boolean value,
// is an integer type; and is either unsigned after implicit casts,
// or was unsigned before implicit casts.
- return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType() &&
+ return isNonBooleanIntegerValue(E) &&
(!E->getType()->isSignedIntegerType() ||
!E->IgnoreParenImpCasts()->getType()->isSignedIntegerType());
}
@@ -8618,6 +8672,88 @@
return Match;
}
+// Does not handle comparison with 0.
+// CheckTautologicalComparisonWithZero() does that already.
+bool CheckTautologicalComparisonWithMinMax(Sema &S, BinaryOperator *E) {
+ // Disable warning in template instantiations.
+ if (S.inTemplateInstantiation())
+ return false;
+
+ // bool values are handled by DiagnoseOutOfRangeComparison().
+ // '0' constant is handled by CheckTautologicalComparisonWithZero().
+
+ BinaryOperatorKind Op = E->getOpcode();
+ if (E->isValueDependent())
+ return false;
+
+ StringRef OpStr = E->getOpcodeStr();
+
+ Expr *LHS = E->getLHS();
+ Expr *RHS = E->getRHS();
+
+ QualType LType = LHS->IgnoreParenImpCasts()->getType();
+ QualType RType = RHS->IgnoreParenImpCasts()->getType();
+
+ bool Match = true;
+ llvm::APSInt Value;
+
+ SmallString<32> PrettySourceValue;
+ llvm::raw_svector_ostream OS(PrettySourceValue);
+
+ if (Op == BO_LT && isNonBooleanIntegerValue(RHS) &&
+ IsTypeLimit(S, RHS, LHS, &Value) == LimitType::Max) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_lconstant_compare)
+ << RType << OpStr << OS.str() << false << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_LT && isNonBooleanIntegerValue(LHS) &&
+ IsTypeLimit(S, LHS, RHS, &Value) == LimitType::Min) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_rconstant_compare)
+ << LType << OpStr << OS.str() << false << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_GE && isNonBooleanIntegerValue(RHS) &&
+ IsTypeLimit(S, RHS, LHS, &Value) == LimitType::Max) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_lconstant_compare)
+ << RType << OpStr << OS.str() << true << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_GE && isNonBooleanIntegerValue(LHS) &&
+ IsTypeLimit(S, LHS, RHS, &Value) == LimitType::Min) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_rconstant_compare)
+ << LType << OpStr << OS.str() << true << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_GT && isNonBooleanIntegerValue(LHS) &&
+ IsTypeLimit(S, LHS, RHS, &Value) == LimitType::Max) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_rconstant_compare)
+ << LType << OpStr << OS.str() << false << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_GT && isNonBooleanIntegerValue(RHS) &&
+ IsTypeLimit(S, RHS, LHS, &Value) == LimitType::Min) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_lconstant_compare)
+ << RType << OpStr << OS.str() << false << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_LE && isNonBooleanIntegerValue(LHS) &&
+ IsTypeLimit(S, LHS, RHS, &Value) == LimitType::Max) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_rconstant_compare)
+ << LType << OpStr << OS.str() << true << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else if (Op == BO_LE && isNonBooleanIntegerValue(RHS) &&
+ IsTypeLimit(S, RHS, LHS, &Value) == LimitType::Min) {
+ OS << Value;
+ S.Diag(E->getOperatorLoc(), diag::warn_tautological_lconstant_compare)
+ << RType << OpStr << OS.str() << true << LHS->getSourceRange()
+ << RHS->getSourceRange();
+ } else
+ Match = false;
+
+ return Match;
+}
+
void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant,
Expr *Other, const llvm::APSInt &Value,
bool RhsConstant) {
@@ -8639,6 +8775,11 @@
if ((Value == 0) && (!OtherIsBooleanType))
return;
+ llvm::APSInt ConstValue;
+ // type limit values are handled later by CheckTautologicalComparisonWithMinMax().
+ if (IsTypeLimit(S, Other, Constant, &ConstValue) && (!OtherIsBooleanType))
+ return;
+
BinaryOperatorKind op = E->getOpcode();
bool IsTrue = true;
@@ -8882,6 +9023,10 @@
if (CheckTautologicalComparisonWithZero(S, E))
return AnalyzeImpConvsInComparison(S, E);
+ // If this is a tautological comparison, suppress -Wsign-compare.
+ if (CheckTautologicalComparisonWithMinMax(S, E))
+ return AnalyzeImpConvsInComparison(S, E);
+
// We don't do anything special if this isn't an unsigned integral
// comparison: we're only interested in integral comparisons, and
// signed comparisons only happen in cases we don't care to warn about.
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5930,6 +5930,12 @@
def warn_mixed_sign_comparison : Warning<
"comparison of integers of different signs: %0 and %1">,
InGroup<SignCompare>, DefaultIgnore;
+def warn_tautological_rconstant_compare : Warning<
+ "comparison %0 %1 %2 is always %select{false|true}3">,
+ InGroup<TautologicalConstantCompare>;
+def warn_tautological_lconstant_compare : Warning<
+ "comparison %2 %1 %0 is always %select{false|true}3">,
+ InGroup<TautologicalConstantCompare>;
def warn_out_of_range_compare : Warning<
"comparison of %select{constant %0|true|false}1 with "
"%select{expression of type %2|boolean expression}3 is always "
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -431,13 +431,15 @@
def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">;
def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">;
def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
+def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
+ [TautologicalUnsignedZeroCompare,
+ TautologicalUnsignedEnumZeroCompare,
+ TautologicalOutOfRangeCompare]>;
def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
def TautologicalCompare : DiagGroup<"tautological-compare",
- [TautologicalUnsignedZeroCompare,
- TautologicalUnsignedEnumZeroCompare,
- TautologicalOutOfRangeCompare,
+ [TautologicalConstantCompare,
TautologicalPointerCompare,
TautologicalOverlapCompare,
TautologicalUndefinedCompare]>;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
