hazohelet created this revision.
hazohelet added reviewers: aaron.ballman, tbaeder, erichkeane.
Herald added a reviewer: NoQ.
Herald added a project: All.
hazohelet requested review of this revision.
Herald added a project: clang.
This patch introduces a new warning flag `-Wtautological-negation-compare`
grouped in `-Wtautological-compare` that warns on the use of `&&` or `||`
operators against a variable and its negation.
e.g. `x || !x` and `!x && x`
This also makes the `-Winfinite-recursion` diagnose more cases.
Fixes https://github.com/llvm/llvm-project/issues/56035
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D152093
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Analysis/CFG.h
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/CFG.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
clang/test/Misc/warning-wall.c
clang/test/SemaCXX/tautological-negation-compare.cpp
clang/test/SemaCXX/warn-infinite-recursion.cpp
Index: clang/test/SemaCXX/warn-infinite-recursion.cpp
===================================================================
--- clang/test/SemaCXX/warn-infinite-recursion.cpp
+++ clang/test/SemaCXX/warn-infinite-recursion.cpp
@@ -203,3 +203,13 @@
(void)typeid(unevaluated_recursive_function());
return 0;
}
+
+void func1(int i) { // expected-warning {{call itself}}
+ if (i || !i)
+ func1(i);
+}
+void func2(int i) { // expected-warning {{call itself}}
+ if (!i && i) {}
+ else
+ func2(i);
+}
Index: clang/test/SemaCXX/tautological-negation-compare.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/tautological-negation-compare.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-negation-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis -Wno-constant-logical-operand %s
+
+#define COPY(x) x
+
+void test_int(int x) {
+ if (x || !x) {} // expected-warning {{'||' against a variable and its negation always evaluates to true}}
+ if (!x || x) {} // expected-warning {{'||' against a variable and its negation always evaluates to true}}
+ if (x && !x) {} // expected-warning {{'&&' against a variable and its negation always evaluates to false}}
+ if (!x && x) {} // expected-warning {{'&&' against a variable and its negation always evaluates to false}}
+
+ // parentheses are ignored
+ if (x || (!x)) {} // expected-warning {{'||' against a variable and its negation always evaluates to true}}
+ if (!(x) || x) {} // expected-warning {{'||' against a variable and its negation always evaluates to true}}
+
+ // don't warn on macros
+ if (COPY(x) || !x) {}
+ if (!x || COPY(x)) {}
+ if (x && COPY(!x)) {}
+ if (COPY(!x && x)) {}
+
+ // dont' warn on literals
+ if (1 || !1) {}
+ if (!42 && 42) {}
+
+
+ // don't warn on overloads
+ struct Foo{
+ int val;
+ Foo operator!() const { return Foo{!val}; }
+ bool operator||(const Foo other) const { return val || other.val; }
+ bool operator&&(const Foo other) const { return val && other.val; }
+ };
+
+ Foo f{3};
+ if (f || !f) {}
+ if (!f || f) {}
+ if (f.val || !f.val) {} // expected-warning {{'||' against a variable and its negation always evaluates to true}}
+ if (!f.val && f.val) {} // expected-warning {{'&&' against a variable and its negation always evaluates to false}}
+}
Index: clang/test/Misc/warning-wall.c
===================================================================
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -55,6 +55,7 @@
CHECK-NEXT: -Wtautological-bitwise-compare
CHECK-NEXT: -Wtautological-undefined-compare
CHECK-NEXT: -Wtautological-objc-bool-compare
+CHECK-NEXT: -Wtautological-negation-compare
CHECK-NEXT: -Wtrigraphs
CHECK-NEXT: -Wuninitialized
CHECK-NEXT: -Wsometimes-uninitialized
Index: clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
===================================================================
--- clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -1393,13 +1393,13 @@
// CHECK: Succs (2): B2 B1
// CHECK: [B4 (NORETURN)]
// CHECK: 1: ~NoReturn() (Temporary object destructor)
-// CHECK: Preds (1): B5
+// CHECK: Preds (1): B5(Unreachable)
// CHECK: Succs (1): B0
// CHECK: [B5]
// CHECK: 1: [B8.3] || [B7.2] || [B6.7]
// CHECK: T: (Temp Dtor) [B6.4]
// CHECK: Preds (3): B6 B7 B8
-// CHECK: Succs (2): B4 B3
+// CHECK: Succs (2): B4(Unreachable) B3
// CHECK: [B6]
// CHECK: 1: check
// CHECK: 2: [B6.1] (ImplicitCastExpr, FunctionToPointerDecay, _Bool (*)(const NoReturn &))
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -158,6 +158,17 @@
return false;
}
+ void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
+ if (HasMacroID(B))
+ return;
+
+ unsigned DiagID = isAlwaysTrue
+ ? diag::warn_tautological_negation_or_compare
+ : diag::warn_tautological_negation_and_compare;
+ SourceRange DiagRange = B->getSourceRange();
+ S.Diag(B->getExprLoc(), DiagID) << DiagRange;
+ }
+
void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
if (HasMacroID(B))
return;
@@ -188,7 +199,8 @@
static bool hasActiveDiagnostics(DiagnosticsEngine &Diags,
SourceLocation Loc) {
return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
- !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+ !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc) ||
+ !Diags.isIgnored(diag::warn_tautological_negation_and_compare, Loc);
}
};
} // anonymous namespace
Index: clang/lib/Analysis/CFG.cpp
===================================================================
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -1077,16 +1077,41 @@
}
}
- /// Find a pair of comparison expressions with or without parentheses
+ /// There are two checks handled by this function:
+ /// 1. Find a law-of-excluded-middle or law-of-noncontradiction expression
+ /// e.g. if (x || !x), if (x && !x)
+ /// 2. Find a pair of comparison expressions with or without parentheses
/// with a shared variable and constants and a logical operator between them
/// that always evaluates to either true or false.
/// e.g. if (x != 3 || x != 4)
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
assert(B->isLogicalOp());
- const BinaryOperator *LHS =
- dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
- const BinaryOperator *RHS =
- dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
+ const Expr *LHSExpr = B->getLHS()->IgnoreParens();
+ const Expr *RHSExpr = B->getRHS()->IgnoreParens();
+
+ auto CheckLogicalOpWithNegatedVariable = [this, B](const Expr *E1,
+ const Expr *E2) {
+ if (const auto *Negate = dyn_cast<UnaryOperator>(E1)) {
+ if (Negate->getOpcode() == UO_LNot &&
+ Expr::isSameComparisonOperand(Negate->getSubExpr(), E2)) {
+ bool AlwaysTrue = B->getOpcode() == BO_LOr;
+ if (BuildOpts.Observer)
+ BuildOpts.Observer->logicAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ }
+ return TryResult();
+ };
+
+ TryResult Result = CheckLogicalOpWithNegatedVariable(LHSExpr, RHSExpr);
+ if (Result.isKnown())
+ return Result;
+ Result = CheckLogicalOpWithNegatedVariable(RHSExpr, LHSExpr);
+ if (Result.isKnown())
+ return Result;
+
+ const BinaryOperator *LHS = dyn_cast<BinaryOperator>(LHSExpr);
+ const BinaryOperator *RHS = dyn_cast<BinaryOperator>(RHSExpr);
if (!LHS || !RHS)
return {};
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9723,6 +9723,12 @@
def warn_comparison_bitwise_or : Warning<
"bitwise or with non-zero value always evaluates to true">,
InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
+def warn_tautological_negation_and_compare: Warning<
+ "'&&' against a variable and its negation always evaluates to false">,
+ InGroup<TautologicalNegationCompare>, DefaultIgnore;
+def warn_tautological_negation_or_compare: Warning<
+ "'||' against a variable and its negation always evaluates to true">,
+ InGroup<TautologicalNegationCompare>, DefaultIgnore;
def warn_tautological_overlap_comparison : Warning<
"overlapping comparisons always evaluate to %select{false|true}0">,
InGroup<TautologicalOverlapCompare>, DefaultIgnore;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -667,13 +667,15 @@
def TautologicalBitwiseCompare : DiagGroup<"tautological-bitwise-compare">;
def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
+def TautologicalNegationCompare : DiagGroup<"tautological-negation-compare">;
def TautologicalCompare : DiagGroup<"tautological-compare",
[TautologicalConstantCompare,
TautologicalPointerCompare,
TautologicalOverlapCompare,
TautologicalBitwiseCompare,
TautologicalUndefinedCompare,
- TautologicalObjCBoolCompare]>;
+ TautologicalObjCBoolCompare,
+ TautologicalNegationCompare]>;
def HeaderHygiene : DiagGroup<"header-hygiene">;
def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
Index: clang/include/clang/Analysis/CFG.h
===================================================================
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1209,6 +1209,7 @@
CFGCallback() = default;
virtual ~CFGCallback() = default;
+ virtual void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
virtual void compareBitwiseEquality(const BinaryOperator *B,
bool isAlwaysTrue) {}
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -319,6 +319,10 @@
``-fno-diagnostics-show-line-numbers``. At the same time, the maximum
number of code lines it prints has been increased from 1 to 16. This
can be controlled using ``-fcaret-diagnostics-max-lines=``.
+- Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
+ tautologies like ``x && !x`` and ``!x || x`` in expressions. This also
+ makes ``-Winfinite-recursion`` diagnose more cases.
+ (`#56035: <https://github.com/llvm/llvm-project/issues/56035>`_).
Bug Fixes in This Version
-------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
