lebedev.ri updated this revision to Diff 121044.
lebedev.ri added a comment.
In https://reviews.llvm.org/D39462#912011, @rjmccall wrote:
> The actual choice of integer type is not stable across targets any more than
> the size is. In practice, people often don't use int and long, they use
> standard typedefs like size_t and uint64_t, but the actual type chosen for
> size_t is arbitrary when there are multiple types at that bit-width.
The internal canonical types are compared, so all this `typedef` sugar will be
silently ignored.
> So I'm concerned that you're suppressing more than you think here and still
> not going to satisfy people.
Possibly...
But it will at least make it easier to deal with the most common pattern "that
obviously should not warn".
I'd love to hear better ideas
In https://reviews.llvm.org/D39462#912011, @rjmccall wrote:
> Also, "data model" is not a term in use. Use "target" or "target platform".
Changed to `for the current target platform, ...`
Repository:
rL LLVM
https://reviews.llvm.org/D39462
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/Sema/maybe-tautological-constant-compare.c
Index: test/Sema/maybe-tautological-constant-compare.c
===================================================================
--- /dev/null
+++ test/Sema/maybe-tautological-constant-compare.c
@@ -0,0 +1,342 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DLP64 -Wmaybe-tautological-constant-compare -DTEST -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DLP64 -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DLP64 -Wmaybe-tautological-constant-compare -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DLP64 -verify -x c++ %s
+// RUN: %clang_cc1 -triple i386-linux-gnu -fsyntax-only -DILP32 -Wmaybe-tautological-constant-compare -DTEST -verify %s
+// RUN: %clang_cc1 -triple i386-linux-gnu -fsyntax-only -DILP32 -verify %s
+// R1UN: %clang_cc1 -triple i386-linux-gnu -fsyntax-only -DILP32 -Wmaybe-tautological-constant-compare -DTEST -verify -x c++ %s
+// R1UN: %clang_cc1 -triple i386-linux-gnu -fsyntax-only -DILP32 -verify -x c++ %s
+
+int value(void);
+
+int main() {
+ unsigned long ul = value();
+
+#if defined(LP64)
+#if __SIZEOF_INT__ != 4
+#error Expecting 32-bit int
+#endif
+#if __SIZEOF_LONG__ != 8
+#error Expecting 64-bit int
+#endif
+#elif defined(ILP32)
+#if __SIZEOF_INT__ != 4
+#error Expecting 32-bit int
+#endif
+#if __SIZEOF_LONG__ != 4
+#error Expecting 32-bit int
+#endif
+#else
+#error LP64 or ILP32 should be defined
+#endif
+
+#if defined(LP64)
+ // expected-no-diagnostics
+
+ if (ul < 4294967295)
+ return 0;
+ if (4294967295 >= ul)
+ return 0;
+ if (ul > 4294967295)
+ return 0;
+ if (4294967295 <= ul)
+ return 0;
+ if (ul <= 4294967295)
+ return 0;
+ if (4294967295 > ul)
+ return 0;
+ if (ul >= 4294967295)
+ return 0;
+ if (4294967295 < ul)
+ return 0;
+ if (ul == 4294967295)
+ return 0;
+ if (4294967295 != ul)
+ return 0;
+ if (ul != 4294967295)
+ return 0;
+ if (4294967295 == ul)
+ return 0;
+
+ if (ul < 4294967295U)
+ return 0;
+ if (4294967295U >= ul)
+ return 0;
+ if (ul > 4294967295U)
+ return 0;
+ if (4294967295U <= ul)
+ return 0;
+ if (ul <= 4294967295U)
+ return 0;
+ if (4294967295U > ul)
+ return 0;
+ if (ul >= 4294967295U)
+ return 0;
+ if (4294967295U < ul)
+ return 0;
+ if (ul == 4294967295U)
+ return 0;
+ if (4294967295U != ul)
+ return 0;
+ if (ul != 4294967295U)
+ return 0;
+ if (4294967295U == ul)
+ return 0;
+
+ if (ul < 4294967295L)
+ return 0;
+ if (4294967295L >= ul)
+ return 0;
+ if (ul > 4294967295L)
+ return 0;
+ if (4294967295L <= ul)
+ return 0;
+ if (ul <= 4294967295L)
+ return 0;
+ if (4294967295L > ul)
+ return 0;
+ if (ul >= 4294967295L)
+ return 0;
+ if (4294967295L < ul)
+ return 0;
+ if (ul == 4294967295L)
+ return 0;
+ if (4294967295L != ul)
+ return 0;
+ if (ul != 4294967295L)
+ return 0;
+ if (4294967295L == ul)
+ return 0;
+
+ if (ul < 4294967295UL)
+ return 0;
+ if (4294967295UL >= ul)
+ return 0;
+ if (ul > 4294967295UL)
+ return 0;
+ if (4294967295UL <= ul)
+ return 0;
+ if (ul <= 4294967295UL)
+ return 0;
+ if (4294967295UL > ul)
+ return 0;
+ if (ul >= 4294967295UL)
+ return 0;
+ if (4294967295UL < ul)
+ return 0;
+ if (ul == 4294967295UL)
+ return 0;
+ if (4294967295UL != ul)
+ return 0;
+ if (ul != 4294967295UL)
+ return 0;
+ if (4294967295UL == ul)
+ return 0;
+#elif defined(ILP32)
+#if defined(TEST)
+ if (ul < 4294967295)
+ return 0;
+ if (4294967295 >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295 <= ul)
+ return 0;
+ if (ul <= 4294967295) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295 > ul)
+ return 0;
+ if (ul >= 4294967295)
+ return 0;
+ if (4294967295 < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295)
+ return 0;
+ if (4294967295 != ul)
+ return 0;
+ if (ul != 4294967295)
+ return 0;
+ if (4294967295 == ul)
+ return 0;
+
+ if (ul < 4294967295U)
+ return 0;
+ if (4294967295U >= ul) // expected-warning {{for the current target platform, comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295U) // expected-warning {{for the current target platform, comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295U <= ul)
+ return 0;
+ if (ul <= 4294967295U) // expected-warning {{for the current target platform, comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295U > ul)
+ return 0;
+ if (ul >= 4294967295U)
+ return 0;
+ if (4294967295U < ul) // expected-warning {{for the current target platform, comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295U)
+ return 0;
+ if (4294967295U != ul)
+ return 0;
+ if (ul != 4294967295U)
+ return 0;
+ if (4294967295U == ul)
+ return 0;
+
+ if (ul < 4294967295L)
+ return 0;
+ if (4294967295L >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295L) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295L <= ul)
+ return 0;
+ if (ul <= 4294967295L) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295L > ul)
+ return 0;
+ if (ul >= 4294967295L)
+ return 0;
+ if (4294967295L < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295L)
+ return 0;
+ if (4294967295L != ul)
+ return 0;
+ if (ul != 4294967295L)
+ return 0;
+ if (4294967295L == ul)
+ return 0;
+
+ if (ul < 4294967295UL)
+ return 0;
+ if (4294967295UL >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295UL) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295UL <= ul)
+ return 0;
+ if (ul <= 4294967295UL) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295UL > ul)
+ return 0;
+ if (ul >= 4294967295UL)
+ return 0;
+ if (4294967295UL < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295UL)
+ return 0;
+ if (4294967295UL != ul)
+ return 0;
+ if (ul != 4294967295UL)
+ return 0;
+ if (4294967295UL == ul)
+ return 0;
+#else // defined(TEST)
+ if (ul < 4294967295)
+ return 0;
+ if (4294967295 >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295 <= ul)
+ return 0;
+ if (ul <= 4294967295) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295 > ul)
+ return 0;
+ if (ul >= 4294967295)
+ return 0;
+ if (4294967295 < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295)
+ return 0;
+ if (4294967295 != ul)
+ return 0;
+ if (ul != 4294967295)
+ return 0;
+ if (4294967295 == ul)
+ return 0;
+
+ if (ul < 4294967295U)
+ return 0;
+ if (4294967295U >= ul)
+ return 0;
+ if (ul > 4294967295U)
+ return 0;
+ if (4294967295U <= ul)
+ return 0;
+ if (ul <= 4294967295U)
+ return 0;
+ if (4294967295U > ul)
+ return 0;
+ if (ul >= 4294967295U)
+ return 0;
+ if (4294967295U < ul)
+ return 0;
+ if (ul == 4294967295U)
+ return 0;
+ if (4294967295U != ul)
+ return 0;
+ if (ul != 4294967295U)
+ return 0;
+ if (4294967295U == ul)
+ return 0;
+
+ if (ul < 4294967295L)
+ return 0;
+ if (4294967295L >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295L) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295L <= ul)
+ return 0;
+ if (ul <= 4294967295L) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295L > ul)
+ return 0;
+ if (ul >= 4294967295L)
+ return 0;
+ if (4294967295L < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295L)
+ return 0;
+ if (4294967295L != ul)
+ return 0;
+ if (ul != 4294967295L)
+ return 0;
+ if (4294967295L == ul)
+ return 0;
+
+ if (ul < 4294967295UL)
+ return 0;
+ if (4294967295UL >= ul) // expected-warning {{comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295UL) // expected-warning {{comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295UL <= ul)
+ return 0;
+ if (ul <= 4294967295UL) // expected-warning {{comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295UL > ul)
+ return 0;
+ if (ul >= 4294967295UL)
+ return 0;
+ if (4294967295UL < ul) // expected-warning {{comparison 4294967295 < 'unsigned long' is always false}}
+ return 0;
+ if (ul == 4294967295UL)
+ return 0;
+ if (4294967295UL != ul)
+ return 0;
+ if (ul != 4294967295UL)
+ return 0;
+ if (4294967295UL == ul)
+ return 0;
+#endif // defined(TEST)
+#else
+#error LP64 or ILP32 should be defined
+#endif
+
+ return 1;
+}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8159,6 +8159,11 @@
: Width(Width), NonNegative(NonNegative)
{}
+ /// Indicate whether the specified integer ranges are identical.
+ friend bool operator==(const IntRange &LHS, const IntRange &RHS) {
+ return LHS.Width == RHS.Width && LHS.NonNegative == RHS.NonNegative;
+ }
+
/// Returns the range of the bool type.
static IntRange forBoolType() {
return IntRange(1, true);
@@ -8588,12 +8593,42 @@
}
enum class LimitType {
- 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 };
+ None = 0, // Just a zero-initalizer
+ 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 };
+ DataModelDependent = 1U << 2U, // is this limit is always the limit, or only
+ // for the current data model
};
+/// LimitType is a bitset, thus a few helpers are needed
+LimitType operator|(LimitType LHS, LimitType RHS) {
+ return static_cast<LimitType>(
+ static_cast<std::underlying_type<LimitType>::type>(LHS) |
+ static_cast<std::underlying_type<LimitType>::type>(RHS));
+}
+LimitType operator&(LimitType LHS, LimitType RHS) {
+ return static_cast<LimitType>(
+ static_cast<std::underlying_type<LimitType>::type>(LHS) &
+ static_cast<std::underlying_type<LimitType>::type>(RHS));
+}
+bool IsSet(llvm::Optional<LimitType> LHS, LimitType RHS) {
+ return LHS && ((*LHS) & RHS) == RHS;
+}
+
+bool HasEnumType(Expr *E) {
+ // Strip off implicit integral promotions.
+ while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+ if (ICE->getCastKind() != CK_IntegralCast &&
+ ICE->getCastKind() != CK_NoOp)
+ break;
+ E = ICE->getSubExpr();
+ }
+
+ return E->getType()->isEnumeralType();
+}
+
/// 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).
@@ -8608,43 +8643,36 @@
// 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();
-
+ QualType ConstT =
+ Constant->IgnoreParenImpCasts()->getType()->getCanonicalTypeInternal();
+ QualType OtherT =
+ Other->IgnoreParenImpCasts()->getType()->getCanonicalTypeInternal();
+ IntRange ConstRange = IntRange::forValueOfType(S.Context, ConstT);
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<LimitType>();
+ LimitType LT = LimitType::None;
+ if(ConstRange == OtherRange && ConstT != OtherT && !HasEnumType(Other))
+ LT = LimitType::DataModelDependent;
+
if (llvm::APSInt::isSameValue(
llvm::APSInt::getMaxValue(OtherRange.Width,
OtherT->isUnsignedIntegerType()),
Value))
- return LimitType::Max;
+ return LimitType::Max | LT;
if (llvm::APSInt::isSameValue(
llvm::APSInt::getMinValue(OtherRange.Width,
OtherT->isUnsignedIntegerType()),
Value))
- return LimitType::Min;
+ return LimitType::Min | LT;
return llvm::None;
}
-bool HasEnumType(Expr *E) {
- // Strip off implicit integral promotions.
- while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
- if (ICE->getCastKind() != CK_IntegralCast &&
- ICE->getCastKind() != CK_NoOp)
- break;
- E = ICE->getSubExpr();
- }
-
- return E->getType()->isEnumeralType();
-}
-
bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, Expr *Constant,
Expr *Other, const llvm::APSInt &Value,
bool RhsConstant) {
@@ -8665,9 +8693,9 @@
bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
- if (ValueType != LimitType::Both) {
+ if (!IsSet(ValueType, LimitType::Both)) {
bool ResultWhenConstNeOther =
- ConstIsLowerBound ^ (ValueType == LimitType::Max);
+ ConstIsLowerBound ^ IsSet(ValueType, LimitType::Max);
if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
return false; // The comparison is not tautological.
} else if (ResultWhenConstEqualsOther == ConstIsLowerBound)
@@ -8679,7 +8707,9 @@
? (HasEnumType(Other)
? diag::warn_unsigned_enum_always_true_comparison
: diag::warn_unsigned_always_true_comparison)
- : diag::warn_tautological_constant_compare;
+ : IsSet(ValueType, LimitType::DataModelDependent)
+ ? diag::warn_maybe_tautological_constant_compare
+ : diag::warn_tautological_constant_compare;
// Should be enough for uint128 (39 decimal digits)
SmallString<64> PrettySourceValue;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5972,6 +5972,10 @@
"comparison %select{%3|%1}0 %2 "
"%select{%1|%3}0 is always %select{false|true}4">,
InGroup<TautologicalConstantCompare>;
+def warn_maybe_tautological_constant_compare : Warning<
+ "for the current target platform, comparison %select{%3|%1}0 %2 "
+ "%select{%1|%3}0 is always %select{false|true}4">,
+ InGroup<MaybeTautologicalConstantCompare>, DefaultIgnore;
def warn_mixed_sign_comparison : Warning<
"comparison of integers of different signs: %0 and %1">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -438,10 +438,12 @@
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 MaybeTautologicalConstantCompare : DiagGroup<"maybe-tautological-constant-compare">;
def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
[TautologicalUnsignedZeroCompare,
TautologicalUnsignedEnumZeroCompare,
- TautologicalOutOfRangeCompare]>;
+ TautologicalOutOfRangeCompare,
+ MaybeTautologicalConstantCompare]>;
def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -81,6 +81,8 @@
- ``-Wtautological-constant-compare`` is a new warning that warns on
tautological comparisons between integer variable of the type ``T`` and the
largest/smallest possible integer constant of that same type.
+ If the tautologicalness of the comparison is data model dependent, then the
+ ``-Wmaybe-tautological-constant-compare`` (disabled by default) is used.
- For C code, ``-Wsign-compare``, ``-Wtautological-constant-compare`` and
``-Wtautological-constant-out-of-range-compare`` were adjusted to use the
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
