And i have again forgot to add the appropriate -commits list from the start.
Is there any reason at all why phab does not/should not do that automatically?
---------- Forwarded message ----------
From: Roman Lebedev via Phabricator <revi...@reviews.llvm.org>
Date: Tue, Oct 31, 2017 at 8:44 PM
Subject: [PATCH] D39462: [Sema] Implement
-Wmaybe-tautological-constant-compare for when the tautologicalness is
data model dependent
To: lebedev...@gmail.com, rich...@metafoo.co.uk, rjmcc...@gmail.com,
dblai...@gmail.com
Cc: mclow.li...@gmail.com, aaron.ball...@gmail.com,
bc...@codeaurora.org, smee...@fb.com, jkor...@apple.com,
shen...@google.com
lebedev.ri created this revision.
lebedev.ri added a project: clang.
As brought up in https://reviews.llvm.org/D39149, and post-review
mails for some other commits,
the current `-Wtautological-constant-compare` is dumb, much like
unreachable code diagnostic.
The common complaint is that it diagnoses the comparisons between an `int` and
`long` when compiling for a 32-bit target as tautological, but not when
compiling for 64-bit targets. The underlying problem is obvious: data model.
In most cases, 64-bit target is `LP64` (`int` is 32-bit, `long` and pointer are
64-bit), and the 32-bit target is `ILP32` (`int`, `long`, and pointer
are 32-bit).
I.e. the common pattern is: (pseudocode)
#include <limits>
#include <cstdint>
int main() {
using T1 = long;
using T2 = int;
T1 r;
if (r < std::numeric_limits<T2>::min()) {}
if (r > std::numeric_limits<T2>::max()) {}
}
The fix is simple: if the types of the values being compared are different, but
are of the same size, then we issue `-Wmaybe-tautological-constant-compare`
instead of `-Wtautological-constant-compare`.
That new diagnostic is *not* enabled by default/-Wall/-Wextra.
Caveat: i did not actually verify that ^ pseudocode actually triggers
the new diag.
Looking at AST, it should.
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 {{with current data model, comparison 4294967295 >= 'unsigned long' is always true}}
+ return 0;
+ if (ul > 4294967295U) // expected-warning {{with current data model, comparison 'unsigned long' > 4294967295 is always false}}
+ return 0;
+ if (4294967295U <= ul)
+ return 0;
+ if (ul <= 4294967295U) // expected-warning {{with current data model, comparison 'unsigned long' <= 4294967295 is always true}}
+ return 0;
+ if (4294967295U > ul)
+ return 0;
+ if (ul >= 4294967295U)
+ return 0;
+ if (4294967295U < ul) // expected-warning {{with current data model, 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<
+ "with current data model, 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
