[clang-tools-extra] [clang-tidy] Address false positives in misc-redundant-expression checker (PR #121960)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 7 08:21:18 PST 2025
https://github.com/earnol created https://github.com/llvm/llvm-project/pull/121960
This patch addresses situations when misc-redundant-expression checker provides excessive diagnostics for situations with different macros having the same value. In particular it addresses situations described in the initial report of https://github.com/llvm/llvm-project/issues/118885 are addressed. The situations which are popped inside discussion like if (A + B == B + A) for macros are not properly addressed by this patch.
>From abd79628aa576709dcbce3c755d1aa98087c0f04 Mon Sep 17 00:00:00 2001
From: Vladislav Aranov <vladislav.aranov at ericsson.com>
Date: Tue, 7 Jan 2025 11:11:44 -0500
Subject: [PATCH] [clang-tidy] Address false positives in
misc-redundant-expression checker
Address situations when misc-redundant-expression checker provides excessive
diagnostics for situations with different macros having the same value.
In perticular situations described in the initial report of
https://github.com/llvm/llvm-project/issues/118885 are addressed.
The situations which are popped inside discussion like
if (A + B == B + A) for macros are not properly addressed by this patch.
But i still believe even in the current state the patch can be useful.
---
.../misc/RedundantExpressionCheck.cpp | 118 +++++++++++---
.../checkers/misc/redundant-expression.cpp | 150 ++++++++++++++++++
2 files changed, 249 insertions(+), 19 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index fc35bc22c52e04..d6136c40d64cc8 100644
--- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -139,10 +139,8 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
return cast<BinaryOperator>(Left)->getOpcode() ==
cast<BinaryOperator>(Right)->getOpcode();
case Stmt::UnaryExprOrTypeTraitExprClass:
- const auto *LeftUnaryExpr =
- cast<UnaryExprOrTypeTraitExpr>(Left);
- const auto *RightUnaryExpr =
- cast<UnaryExprOrTypeTraitExpr>(Right);
+ const auto *LeftUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Left);
+ const auto *RightUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Right);
if (LeftUnaryExpr->isArgumentType() && RightUnaryExpr->isArgumentType())
return LeftUnaryExpr->getKind() == RightUnaryExpr->getKind() &&
LeftUnaryExpr->getArgumentType() ==
@@ -699,7 +697,8 @@ static bool retrieveRelationalIntegerConstantExpr(
Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
OperandExpr = OverloadedOperatorExpr;
- Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
+ Opcode = BinaryOperator::getOverloadedOpcode(
+ OverloadedOperatorExpr->getOperator());
if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
return false;
@@ -728,7 +727,8 @@ static bool retrieveRelationalIntegerConstantExpr(
}
// Checks for expressions like (X == 4) && (Y != 9)
-static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp,
+ const ASTContext *AstCtx) {
const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
@@ -747,6 +747,31 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
return false;
}
+static bool
+areSidesBinaryConstExpressionsOrDefines(const BinaryOperator *&BinOp,
+ const ASTContext *AstCtx) {
+ if (areSidesBinaryConstExpressions(BinOp, AstCtx))
+ return true;
+
+ const auto *Lhs = BinOp->getLHS();
+ const auto *Rhs = BinOp->getRHS();
+
+ if (!Lhs || !Rhs)
+ return false;
+
+ auto IsDefineExpr = [AstCtx](const Expr *E) {
+ SourceRange Lsr = E->getSourceRange();
+ if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(*AstCtx))
+ return false;
+ return true;
+ };
+
+ if (IsDefineExpr(Lhs) || IsDefineExpr(Rhs))
+ return true;
+ return false;
+}
+
// Retrieves integer constant subexpressions from binary operator expressions
// that have two equivalent sides.
// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5.
@@ -785,7 +810,7 @@ static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
}
static bool isSameRawIdentifierToken(const Token &T1, const Token &T2,
- const SourceManager &SM) {
+ const SourceManager &SM) {
if (T1.getKind() != T2.getKind())
return false;
if (T1.isNot(tok::raw_identifier))
@@ -852,6 +877,58 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
return LhsLoc.isMacroID() != RhsLoc.isMacroID();
}
+
+static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
+ const ASTContext *Context) {
+
+ if (!BinOp)
+ return false;
+
+ const auto *Lhs = BinOp->getLHS();
+ const auto *Rhs = BinOp->getRHS();
+ const SourceManager &SM = Context->getSourceManager();
+
+ SourceRange Lsr = Lhs->getSourceRange();
+ SourceRange Rsr = Rhs->getSourceRange();
+ if (Lsr.getBegin().isMacroID()) {
+ // Left is macro so right macro too
+ if (Rsr.getBegin().isMacroID()) {
+ // Both sides are macros so they are same macro or literal
+ llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
+ llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
+ return L.compare(R) == 0;
+ }
+ // Left is macro but right is not so they are not same macro or literal
+ return false;
+ } else {
+ const IntegerLiteral *Lil = dyn_cast<IntegerLiteral>(Lhs);
+ const IntegerLiteral *Ril = dyn_cast<IntegerLiteral>(Rhs);
+ if (Lil && Ril) {
+ return Lil->getValue() == Ril->getValue();
+ }
+
+ const StringLiteral *LStrl = dyn_cast<StringLiteral>(Lhs);
+ const StringLiteral *RStrl = dyn_cast<StringLiteral>(Rhs);
+ if (Lil && Ril) {
+ llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ return L.compare(R) == 0;
+ }
+
+ const CXXBoolLiteralExpr *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
+ const CXXBoolLiteralExpr *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
+ if (Lbl && Rbl) {
+ return Lbl->getValue() == Rbl->getValue();
+ }
+ }
+ return false;
+}
} // namespace
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1166,6 @@ static bool exprEvaluatesToSymbolic(BinaryOperatorKind Opcode, APSInt Value) {
((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
}
-
void RedundantExpressionCheck::checkBitwiseExpr(
const MatchFinder::MatchResult &Result) {
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
@@ -1134,8 +1210,8 @@ void RedundantExpressionCheck::checkBitwiseExpr(
ConstExpr))
return;
- if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
- return;
+ if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
+ return;
SourceLocation Loc = IneffectiveOperator->getOperatorLoc();
@@ -1276,17 +1352,21 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
- if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+ if (areSidesBinaryConstExpressionsOrDefines(BinOp, Result.Context)) {
const Expr *LhsConst = nullptr, *RhsConst = nullptr;
BinaryOperatorKind MainOpcode{}, SideOpcode{};
-
- if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
- LhsConst, RhsConst, Result.Context))
- return;
-
- if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
- areExprsMacroAndNonMacro(LhsConst, RhsConst))
- return;
+ if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+ if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
+ LhsConst, RhsConst, Result.Context))
+ return;
+
+ if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
+ areExprsMacroAndNonMacro(LhsConst, RhsConst))
+ return;
+ } else {
+ if (!areExprsSameMacroOrLiteral(BinOp, Result.Context))
+ return;
+ }
}
diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
index 7396d2dce76c43..75a73808900724 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
@@ -1,4 +1,5 @@
// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing -Wno-array-compare-cxx26
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -fno-delayed-template-parsing -Wno-array-compare-cxx26 -DTEST_MACRO
typedef __INT64_TYPE__ I64;
@@ -91,6 +92,155 @@ int TestSimpleEquivalent(int X, int Y) {
return 0;
}
+#ifndef TEST_MACRO
+#define VAL_1 2
+#define VAL_3 3
+#else
+#define VAL_1 3
+#define VAL_3 2
+#endif
+
+#define VAL_2 2
+
+#ifndef TEST_MACRO
+#define VAL_4 2 + 1
+#define VAL_6 3 + 1
+#else
+#define VAL_4 3 + 1
+#define VAL_6 2 + 1
+#endif
+
+#define VAL_5 2 + 1
+
+struct TestStruct
+{
+ int mA;
+ int mB;
+ int mC[10];
+};
+
+int TestDefineEquivalent() {
+
+ int int_val1 = 3;
+ int int_val2 = 4;
+ int int_val = 0;
+ const int cint_val2 = 4;
+
+ // Cases which should not be reported
+ if (VAL_1 != VAL_2) return 0;
+ if (VAL_3 != VAL_2) return 0;
+ if (VAL_1 == VAL_2) return 0;
+ if (VAL_3 == VAL_2) return 0;
+ if (VAL_1 >= VAL_2) return 0;
+ if (VAL_3 >= VAL_2) return 0;
+ if (VAL_1 <= VAL_2) return 0;
+ if (VAL_3 <= VAL_2) return 0;
+ if (VAL_1 < VAL_2) return 0;
+ if (VAL_3 < VAL_2) return 0;
+ if (VAL_1 > VAL_2) return 0;
+ if (VAL_3 > VAL_2) return 0;
+
+ if (VAL_4 != VAL_5) return 0;
+ if (VAL_6 != VAL_5) return 0;
+ if (VAL_6 == VAL_5) return 0;
+ if (VAL_4 >= VAL_5) return 0;
+ if (VAL_6 >= VAL_5) return 0;
+ if (VAL_4 <= VAL_5) return 0;
+ if (VAL_6 <= VAL_5) return 0;
+ if (VAL_4 > VAL_5) return 0;
+ if (VAL_6 > VAL_5) return 0;
+ if (VAL_4 < VAL_5) return 0;
+ if (VAL_6 < VAL_5) return 0;
+
+ if (VAL_1 != 2) return 0;
+ if (VAL_3 == 3) return 0;
+
+ if (VAL_1 >= VAL_1) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+ if (VAL_2 <= VAL_2) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+ if (VAL_3 > VAL_3) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+ if (VAL_4 < VAL_4) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+ if (VAL_6 == VAL_6) return 2;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+ if (VAL_5 != VAL_5) return 2;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+
+ if (1 >= 1) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
+ if (0xFF <= 0xFF) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both sides of operator are equivalent
+ if (042 > 042) return 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: both sides of operator are equivalent
+
+ int_val = (VAL_6 == VAL_6)?int_val1: int_val2;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
+ int_val = (042 > 042)?int_val1: int_val2;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both sides of operator are equivalent
+
+
+ // Ternary operator cases which should not be reported
+ int_val = (VAL_4 == VAL_5)? int_val1: int_val2;
+ int_val = (VAL_3 != VAL_2)? int_val1: int_val2;
+ int_val = (VAL_6 != 10)? int_val1: int_val2;
+ int_val = (VAL_6 != 3)? int_val1: int_val2;
+ int_val = (VAL_6 != 4)? int_val1: int_val2;
+ int_val = (VAL_6 == 3)? int_val1: int_val2;
+ int_val = (VAL_6 == 4)? int_val1: int_val2;
+
+ TestStruct tsVar1 = {
+ .mA = 3,
+ .mB = int_val,
+ .mC[0 ... VAL_2 - 2] = int_val + 1,
+ };
+
+ TestStruct tsVar2 = {
+ .mA = 3,
+ .mB = int_val,
+ .mC[0 ... cint_val2 - 2] = int_val + 1,
+ };
+
+ TestStruct tsVar3 = {
+ .mA = 3,
+ .mB = int_val,
+ .mC[0 ... VAL_3 - VAL_3] = int_val + 1,
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: both sides of operator are equivalent
+ };
+
+ TestStruct tsVar4 = {
+ .mA = 3,
+ .mB = int_val,
+ .mC[0 ... 5 - 5] = int_val + 1,
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: both sides of operator are equivalent
+ };
+
+ return 1 + int_val + sizeof(tsVar1) + sizeof(tsVar2) +
+ sizeof(tsVar3) + sizeof(tsVar4);
+}
+
+#define LOOP_DEFINE 1
+
+unsigned int testLoops(const unsigned int arr1[LOOP_DEFINE])
+{
+ unsigned int localIndex;
+ for (localIndex = LOOP_DEFINE - 1; localIndex > 0; localIndex--)
+ {
+ }
+ for (localIndex = LOOP_DEFINE - 1; 10 > 10; localIndex--)
+ // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: both sides of operator are equivalent
+ {
+ }
+
+ for (localIndex = LOOP_DEFINE - 1; LOOP_DEFINE > LOOP_DEFINE; localIndex--)
+ // CHECK-MESSAGES: :[[@LINE-1]]:50: warning: both sides of operator are equivalent
+ {
+ }
+
+ return localIndex;
+}
+
template <int DX>
int TestSimpleEquivalentDependent() {
if (DX > 0 && DX > 0) return 1;
More information about the cfe-commits
mailing list