[clang-tidy] Address false positives in misc-redundant-expression checker #122841
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-tidy Author: None (earnol) ChangesThis 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 #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. Full diff: https://github.com/llvm/llvm-project/pull/122841.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index fc35bc22c52e04..31650309ecf8b3 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,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
return false;
}
+static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+ if (areSidesBinaryConstExpressions(BinOp, AstCtx))
+ return true;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+
+ if (!Lhs || !Rhs)
+ return false;
+
+ auto IsDefineExpr = [AstCtx](const Expr *E) {
+ const SourceRange Lsr = E->getSourceRange();
+ if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(*AstCtx))
+ return false;
+ return true;
+ };
+
+ return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
+}
+
// 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 +807,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))
@@ -808,8 +830,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
const ASTContext *AstCtx) {
if (!LhsExpr || !RhsExpr)
return false;
- SourceRange Lsr = LhsExpr->getSourceRange();
- SourceRange Rsr = RhsExpr->getSourceRange();
+ const SourceRange Lsr = LhsExpr->getSourceRange();
+ const SourceRange Rsr = RhsExpr->getSourceRange();
if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
return false;
@@ -847,11 +869,104 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
if (!LhsExpr || !RhsExpr)
return false;
- SourceLocation LhsLoc = LhsExpr->getExprLoc();
- SourceLocation RhsLoc = RhsExpr->getExprLoc();
+ const SourceLocation LhsLoc = LhsExpr->getExprLoc();
+ const SourceLocation RhsLoc = RhsExpr->getExprLoc();
return LhsLoc.isMacroID() != RhsLoc.isMacroID();
}
+
+static bool areStringsSameIgnoreSpaces(const llvm::StringRef *Left,
+ const llvm::StringRef *Right) {
+ if (Left == Right)
+ return true;
+ if (Left->compare(*Right) == 0) {
+ return true;
+ }
+ // Do running index comparison
+ size_t LIdx = 0;
+ size_t RIdx = 0;
+ const char *LData = Left->data();
+ const char *RData = Right->data();
+ while (LIdx < Left->size() && RIdx < Right->size()) {
+
+ // Skip spaces and tabs from both strings
+ while (LIdx < Left->size() && (LData[LIdx] == ' ' || LData[LIdx] == '\t')) {
+ LIdx += 1;
+ }
+ while (RIdx < Right->size() &&
+ (RData[RIdx] == ' ' || RData[RIdx] == '\t')) {
+ RIdx += 1;
+ }
+
+ // If all not tabs and spaces are the same then return true
+ if (LIdx >= Left->size() && RIdx >= Right->size())
+ return true;
+
+ // If any characters differs then return false
+ else if (LData[LIdx] != RData[RIdx])
+ return false;
+
+ // If current char is the same ==> continue search
+ else {
+ LIdx += 1;
+ RIdx += 1;
+ }
+ }
+ // Shortest string is processed: issue a verdict
+ return LIdx >= Left->size() && RIdx >= Right->size();
+}
+
+static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
+ const ASTContext *Context) {
+
+ if (!BinOp)
+ return false;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+ const SourceManager &SM = Context->getSourceManager();
+
+ const SourceRange Lsr = Lhs->getSourceRange();
+ const 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
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
+ return areStringsSameIgnoreSpaces(&L, &R);
+ }
+ // Left is macro but right is not so they are not same macro or literal
+ return false;
+ } else {
+ const auto *Lil = dyn_cast<IntegerLiteral>(Lhs);
+ const auto *Ril = dyn_cast<IntegerLiteral>(Rhs);
+ if (Lil && Ril) {
+ return Lil->getValue() == Ril->getValue();
+ }
+
+ const auto *LStrl = dyn_cast<StringLiteral>(Lhs);
+ const auto *RStrl = dyn_cast<StringLiteral>(Rhs);
+ if (Lil && Ril) {
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ return L.compare(R) == 0;
+ }
+
+ const auto *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
+ const auto *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
+ if (Lbl && Rbl) {
+ return Lbl->getValue() == Rbl->getValue();
+ }
+ }
+ return false;
+}
} // namespace
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1204,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 +1248,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,19 +1390,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
- if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+ if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ 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/docs/clang-tidy/checks/misc/redundant-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
index 44f0772d0bedf9..cea998d39bd73b 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
@@ -19,12 +19,14 @@ Examples:
.. code-block:: c++
- ((x+1) | (x+1)) // (x+1) is redundant
- (p->x == p->x) // always true
- (p->x < p->x) // always false
- (speed - speed + 1 == 12) // speed - speed is always zero
- int b = a | 4 | a // identical expr on both sides
- ((x=1) | (x=1)) // expression is identical
+ ((x+1) | (x+1)) // (x+1) is redundant
+ (p->x == p->x) // always true
+ (p->x < p->x) // always false
+ (speed - speed + 1 == 12) // speed - speed is always zero
+ int b = a | 4 | a // identical expr on both sides
+ ((x=1) | (x=1)) // expression is identical
+ (DEFINE_1 | DEFINE_1) // same macro on the both sides
+ ((DEF_1 + DEF_2) | (DEF_1+DEF_2)) // expressions differ in spaces only
Floats are handled except in the case that NaNs are checked like so:
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..676f5af4be7267 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,198 @@ 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;
+}
+
+#define getValue(a) a
+#define getValueM(a) a
+
+int TestParamDefine() {
+ int ret = 0;
+ ret += getValue(VAL_6) == getValue(2);
+ ret += getValue(VAL_6) == getValue(3);
+ ret += getValue(VAL_5) == getValue(2);
+ ret += getValue(VAL_5) == getValue(3);
+ ret += getValue(1) > getValue( 2);
+ ret += getValue(VAL_1) == getValue(VAL_2);
+ ret += getValue(VAL_1) != getValue(VAL_2);
+ ret += getValue(VAL_1) == getValueM(VAL_1);
+ ret += getValue(VAL_1 + VAL_2) == getValueM(VAL_1 + VAL_2);
+ ret += getValue(1) == getValueM(1);
+ ret += getValue(false) == getValueM(false);
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1 );
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue( 1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: both sides of operator are equivalent
+ ret += getValue( 1 ) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue(VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_5) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1+VAL_2) > getValue(VAL_1 + VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1)+getValue(VAL_2) > getValue(VAL_1) + getValue( VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: both sides of operator are equivalent
+ ret += (getValue(VAL_1)+getValue(VAL_2)) > (getValue(VAL_1) + getValue( VAL_2) ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: both sides of operator are equivalent
+ return ret;
+}
+
template <int DX>
int TestSimpleEquivalentDependent() {
if (DX > 0 && DX > 0) return 1;
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9eeb872aa57d79..43efc19757edae 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1250,6 +1250,9 @@ Improvements
- Improved the handling of the ``ownership_returns`` attribute. Now, Clang reports an
error if the attribute is attached to a function that returns a non-pointer value.
Fixes (#GH99501)
+- Improved ``misc-redundant-expression`` checker potentially reducing number of false
+ positive detects where same value might have appeared after macro substitution in
+ binary operator.
Moved checkers
^^^^^^^^^^^^^^
|
Member
|
@llvm/pr-subscribers-clang Author: None (earnol) ChangesThis 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 #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. Full diff: https://github.com/llvm/llvm-project/pull/122841.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index fc35bc22c52e04..31650309ecf8b3 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,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
return false;
}
+static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+ if (areSidesBinaryConstExpressions(BinOp, AstCtx))
+ return true;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+
+ if (!Lhs || !Rhs)
+ return false;
+
+ auto IsDefineExpr = [AstCtx](const Expr *E) {
+ const SourceRange Lsr = E->getSourceRange();
+ if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(*AstCtx))
+ return false;
+ return true;
+ };
+
+ return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
+}
+
// 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 +807,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))
@@ -808,8 +830,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
const ASTContext *AstCtx) {
if (!LhsExpr || !RhsExpr)
return false;
- SourceRange Lsr = LhsExpr->getSourceRange();
- SourceRange Rsr = RhsExpr->getSourceRange();
+ const SourceRange Lsr = LhsExpr->getSourceRange();
+ const SourceRange Rsr = RhsExpr->getSourceRange();
if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
return false;
@@ -847,11 +869,104 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
if (!LhsExpr || !RhsExpr)
return false;
- SourceLocation LhsLoc = LhsExpr->getExprLoc();
- SourceLocation RhsLoc = RhsExpr->getExprLoc();
+ const SourceLocation LhsLoc = LhsExpr->getExprLoc();
+ const SourceLocation RhsLoc = RhsExpr->getExprLoc();
return LhsLoc.isMacroID() != RhsLoc.isMacroID();
}
+
+static bool areStringsSameIgnoreSpaces(const llvm::StringRef *Left,
+ const llvm::StringRef *Right) {
+ if (Left == Right)
+ return true;
+ if (Left->compare(*Right) == 0) {
+ return true;
+ }
+ // Do running index comparison
+ size_t LIdx = 0;
+ size_t RIdx = 0;
+ const char *LData = Left->data();
+ const char *RData = Right->data();
+ while (LIdx < Left->size() && RIdx < Right->size()) {
+
+ // Skip spaces and tabs from both strings
+ while (LIdx < Left->size() && (LData[LIdx] == ' ' || LData[LIdx] == '\t')) {
+ LIdx += 1;
+ }
+ while (RIdx < Right->size() &&
+ (RData[RIdx] == ' ' || RData[RIdx] == '\t')) {
+ RIdx += 1;
+ }
+
+ // If all not tabs and spaces are the same then return true
+ if (LIdx >= Left->size() && RIdx >= Right->size())
+ return true;
+
+ // If any characters differs then return false
+ else if (LData[LIdx] != RData[RIdx])
+ return false;
+
+ // If current char is the same ==> continue search
+ else {
+ LIdx += 1;
+ RIdx += 1;
+ }
+ }
+ // Shortest string is processed: issue a verdict
+ return LIdx >= Left->size() && RIdx >= Right->size();
+}
+
+static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
+ const ASTContext *Context) {
+
+ if (!BinOp)
+ return false;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+ const SourceManager &SM = Context->getSourceManager();
+
+ const SourceRange Lsr = Lhs->getSourceRange();
+ const 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
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
+ return areStringsSameIgnoreSpaces(&L, &R);
+ }
+ // Left is macro but right is not so they are not same macro or literal
+ return false;
+ } else {
+ const auto *Lil = dyn_cast<IntegerLiteral>(Lhs);
+ const auto *Ril = dyn_cast<IntegerLiteral>(Rhs);
+ if (Lil && Ril) {
+ return Lil->getValue() == Ril->getValue();
+ }
+
+ const auto *LStrl = dyn_cast<StringLiteral>(Lhs);
+ const auto *RStrl = dyn_cast<StringLiteral>(Rhs);
+ if (Lil && Ril) {
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ return L.compare(R) == 0;
+ }
+
+ const auto *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
+ const auto *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
+ if (Lbl && Rbl) {
+ return Lbl->getValue() == Rbl->getValue();
+ }
+ }
+ return false;
+}
} // namespace
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1204,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 +1248,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,19 +1390,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
- if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+ if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ 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/docs/clang-tidy/checks/misc/redundant-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
index 44f0772d0bedf9..cea998d39bd73b 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
@@ -19,12 +19,14 @@ Examples:
.. code-block:: c++
- ((x+1) | (x+1)) // (x+1) is redundant
- (p->x == p->x) // always true
- (p->x < p->x) // always false
- (speed - speed + 1 == 12) // speed - speed is always zero
- int b = a | 4 | a // identical expr on both sides
- ((x=1) | (x=1)) // expression is identical
+ ((x+1) | (x+1)) // (x+1) is redundant
+ (p->x == p->x) // always true
+ (p->x < p->x) // always false
+ (speed - speed + 1 == 12) // speed - speed is always zero
+ int b = a | 4 | a // identical expr on both sides
+ ((x=1) | (x=1)) // expression is identical
+ (DEFINE_1 | DEFINE_1) // same macro on the both sides
+ ((DEF_1 + DEF_2) | (DEF_1+DEF_2)) // expressions differ in spaces only
Floats are handled except in the case that NaNs are checked like so:
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..676f5af4be7267 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,198 @@ 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;
+}
+
+#define getValue(a) a
+#define getValueM(a) a
+
+int TestParamDefine() {
+ int ret = 0;
+ ret += getValue(VAL_6) == getValue(2);
+ ret += getValue(VAL_6) == getValue(3);
+ ret += getValue(VAL_5) == getValue(2);
+ ret += getValue(VAL_5) == getValue(3);
+ ret += getValue(1) > getValue( 2);
+ ret += getValue(VAL_1) == getValue(VAL_2);
+ ret += getValue(VAL_1) != getValue(VAL_2);
+ ret += getValue(VAL_1) == getValueM(VAL_1);
+ ret += getValue(VAL_1 + VAL_2) == getValueM(VAL_1 + VAL_2);
+ ret += getValue(1) == getValueM(1);
+ ret += getValue(false) == getValueM(false);
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1 );
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue( 1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: both sides of operator are equivalent
+ ret += getValue( 1 ) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue(VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_5) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1+VAL_2) > getValue(VAL_1 + VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1)+getValue(VAL_2) > getValue(VAL_1) + getValue( VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: both sides of operator are equivalent
+ ret += (getValue(VAL_1)+getValue(VAL_2)) > (getValue(VAL_1) + getValue( VAL_2) ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: both sides of operator are equivalent
+ return ret;
+}
+
template <int DX>
int TestSimpleEquivalentDependent() {
if (DX > 0 && DX > 0) return 1;
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9eeb872aa57d79..43efc19757edae 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1250,6 +1250,9 @@ Improvements
- Improved the handling of the ``ownership_returns`` attribute. Now, Clang reports an
error if the attribute is attached to a function that returns a non-pointer value.
Fixes (#GH99501)
+- Improved ``misc-redundant-expression`` checker potentially reducing number of false
+ positive detects where same value might have appeared after macro substitution in
+ binary operator.
Moved checkers
^^^^^^^^^^^^^^
|
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: None (earnol) ChangesThis 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 #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. Full diff: https://github.com/llvm/llvm-project/pull/122841.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index fc35bc22c52e04..31650309ecf8b3 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,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
return false;
}
+static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+ if (areSidesBinaryConstExpressions(BinOp, AstCtx))
+ return true;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+
+ if (!Lhs || !Rhs)
+ return false;
+
+ auto IsDefineExpr = [AstCtx](const Expr *E) {
+ const SourceRange Lsr = E->getSourceRange();
+ if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(*AstCtx))
+ return false;
+ return true;
+ };
+
+ return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
+}
+
// 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 +807,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))
@@ -808,8 +830,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
const ASTContext *AstCtx) {
if (!LhsExpr || !RhsExpr)
return false;
- SourceRange Lsr = LhsExpr->getSourceRange();
- SourceRange Rsr = RhsExpr->getSourceRange();
+ const SourceRange Lsr = LhsExpr->getSourceRange();
+ const SourceRange Rsr = RhsExpr->getSourceRange();
if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
return false;
@@ -847,11 +869,104 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
if (!LhsExpr || !RhsExpr)
return false;
- SourceLocation LhsLoc = LhsExpr->getExprLoc();
- SourceLocation RhsLoc = RhsExpr->getExprLoc();
+ const SourceLocation LhsLoc = LhsExpr->getExprLoc();
+ const SourceLocation RhsLoc = RhsExpr->getExprLoc();
return LhsLoc.isMacroID() != RhsLoc.isMacroID();
}
+
+static bool areStringsSameIgnoreSpaces(const llvm::StringRef *Left,
+ const llvm::StringRef *Right) {
+ if (Left == Right)
+ return true;
+ if (Left->compare(*Right) == 0) {
+ return true;
+ }
+ // Do running index comparison
+ size_t LIdx = 0;
+ size_t RIdx = 0;
+ const char *LData = Left->data();
+ const char *RData = Right->data();
+ while (LIdx < Left->size() && RIdx < Right->size()) {
+
+ // Skip spaces and tabs from both strings
+ while (LIdx < Left->size() && (LData[LIdx] == ' ' || LData[LIdx] == '\t')) {
+ LIdx += 1;
+ }
+ while (RIdx < Right->size() &&
+ (RData[RIdx] == ' ' || RData[RIdx] == '\t')) {
+ RIdx += 1;
+ }
+
+ // If all not tabs and spaces are the same then return true
+ if (LIdx >= Left->size() && RIdx >= Right->size())
+ return true;
+
+ // If any characters differs then return false
+ else if (LData[LIdx] != RData[RIdx])
+ return false;
+
+ // If current char is the same ==> continue search
+ else {
+ LIdx += 1;
+ RIdx += 1;
+ }
+ }
+ // Shortest string is processed: issue a verdict
+ return LIdx >= Left->size() && RIdx >= Right->size();
+}
+
+static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
+ const ASTContext *Context) {
+
+ if (!BinOp)
+ return false;
+
+ const Expr *Lhs = BinOp->getLHS();
+ const Expr *Rhs = BinOp->getRHS();
+ const SourceManager &SM = Context->getSourceManager();
+
+ const SourceRange Lsr = Lhs->getSourceRange();
+ const 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
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
+ return areStringsSameIgnoreSpaces(&L, &R);
+ }
+ // Left is macro but right is not so they are not same macro or literal
+ return false;
+ } else {
+ const auto *Lil = dyn_cast<IntegerLiteral>(Lhs);
+ const auto *Ril = dyn_cast<IntegerLiteral>(Rhs);
+ if (Lil && Ril) {
+ return Lil->getValue() == Ril->getValue();
+ }
+
+ const auto *LStrl = dyn_cast<StringLiteral>(Lhs);
+ const auto *RStrl = dyn_cast<StringLiteral>(Rhs);
+ if (Lil && Ril) {
+ const llvm::StringRef L = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ const llvm::StringRef R = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
+ LangOptions(), 0);
+ return L.compare(R) == 0;
+ }
+
+ const auto *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
+ const auto *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
+ if (Lbl && Rbl) {
+ return Lbl->getValue() == Rbl->getValue();
+ }
+ }
+ return false;
+}
} // namespace
void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1204,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 +1248,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,19 +1390,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
- if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+ if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+ 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/docs/clang-tidy/checks/misc/redundant-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
index 44f0772d0bedf9..cea998d39bd73b 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst
@@ -19,12 +19,14 @@ Examples:
.. code-block:: c++
- ((x+1) | (x+1)) // (x+1) is redundant
- (p->x == p->x) // always true
- (p->x < p->x) // always false
- (speed - speed + 1 == 12) // speed - speed is always zero
- int b = a | 4 | a // identical expr on both sides
- ((x=1) | (x=1)) // expression is identical
+ ((x+1) | (x+1)) // (x+1) is redundant
+ (p->x == p->x) // always true
+ (p->x < p->x) // always false
+ (speed - speed + 1 == 12) // speed - speed is always zero
+ int b = a | 4 | a // identical expr on both sides
+ ((x=1) | (x=1)) // expression is identical
+ (DEFINE_1 | DEFINE_1) // same macro on the both sides
+ ((DEF_1 + DEF_2) | (DEF_1+DEF_2)) // expressions differ in spaces only
Floats are handled except in the case that NaNs are checked like so:
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..676f5af4be7267 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,198 @@ 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;
+}
+
+#define getValue(a) a
+#define getValueM(a) a
+
+int TestParamDefine() {
+ int ret = 0;
+ ret += getValue(VAL_6) == getValue(2);
+ ret += getValue(VAL_6) == getValue(3);
+ ret += getValue(VAL_5) == getValue(2);
+ ret += getValue(VAL_5) == getValue(3);
+ ret += getValue(1) > getValue( 2);
+ ret += getValue(VAL_1) == getValue(VAL_2);
+ ret += getValue(VAL_1) != getValue(VAL_2);
+ ret += getValue(VAL_1) == getValueM(VAL_1);
+ ret += getValue(VAL_1 + VAL_2) == getValueM(VAL_1 + VAL_2);
+ ret += getValue(1) == getValueM(1);
+ ret += getValue(false) == getValueM(false);
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1 );
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue(1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: both sides of operator are equivalent
+ ret += getValue( 1) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: both sides of operator are equivalent
+ ret += getValue( 1 ) > getValue( 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue(VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue( VAL_5 ) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_5) > getValue( VAL_5 ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1+VAL_2) > getValue(VAL_1 + VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: both sides of operator are equivalent
+ ret += getValue(VAL_1)+getValue(VAL_2) > getValue(VAL_1) + getValue( VAL_2) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: both sides of operator are equivalent
+ ret += (getValue(VAL_1)+getValue(VAL_2)) > (getValue(VAL_1) + getValue( VAL_2) ) ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: both sides of operator are equivalent
+ return ret;
+}
+
template <int DX>
int TestSimpleEquivalentDependent() {
if (DX > 0 && DX > 0) return 1;
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9eeb872aa57d79..43efc19757edae 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1250,6 +1250,9 @@ Improvements
- Improved the handling of the ``ownership_returns`` attribute. Now, Clang reports an
error if the attribute is attached to a function that returns a non-pointer value.
Fixes (#GH99501)
+- Improved ``misc-redundant-expression`` checker potentially reducing number of false
+ positive detects where same value might have appeared after macro substitution in
+ binary operator.
Moved checkers
^^^^^^^^^^^^^^
|
earnol
force-pushed
the
main
branch
4 times, most recently
from
5fd0033 to
545d6ae
Compare
earnol
requested review from
5chmidti,
AaronBallman,
AdamMagierFOSS,
AmrDeveloper,
EugeneZelenko,
PiotrZSL,
mzyKi and
vabridgers
earnol
force-pushed
the
main
branch
4 times, most recently
from
1e0f301 to
9f658b5
Compare
Contributor
Author
|
A gentle reminder to request a review for this PR since it is a week without a review. |
PiotrZSL
reviewed
Jan 20, 2025
|
|
||
| const SourceRange Lsr = Lhs->getSourceRange(); | ||
| const SourceRange Rsr = Rhs->getSourceRange(); | ||
| if (Lsr.getBegin().isMacroID()) { |
Member
There was a problem hiding this comment.
problem with this is that you do not know if every part of LHS or RHS is an macro, because you checking only source of first token. If I would write: +VALUE, where #define VALUE 10, then it would fail.
I think that there is some function in /utils/ that may help here
Contributor
Author
There was a problem hiding this comment.
Checking expressions like +VALUE was not a goal of this function in the first place.
But i agree the case like if(+VALUE == +VALUE) is redundant and should be properly detected. I'll add it to lit tests and make sure it will get detected.
Contributor
Author
There was a problem hiding this comment.
I have added extra lit tests to cover this situation. Please check it out.
…cker 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 llvm#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.
Contributor
Author
|
All comments had been addressed, are there any additional suggestions? |
Replace 'removed' with 'fixed'. Co-authored-by: EugeneZelenko <[email protected]>
Empty line separator added after the merge.
Contributor
Author
|
Hello maintainers, |
HerrCai0907
approved these changes
Feb 9, 2025
Contributor
HerrCai0907
left a comment
HerrCai0907
left a comment
There was a problem hiding this comment.
lgtm.
could you test the case
＃define MACRO "bbb"
"aaa" MACRO == "aaabbb"
Contributor
Author
Yeah. A good suggestions. I expect it to come as a negative as it is now. But Should we really care about this case? |
Contributor
Author
I'll create a separate PR for this case and also include test of ## operator in in test cases. It looks like this one is also missing. |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…cker (llvm#122841) 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 llvm#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. Those changes are also mentioned in Release Notes. --------- Co-authored-by: Vladislav Aranov <[email protected]> Co-authored-by: EugeneZelenko <[email protected]>
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
…cker (llvm#122841) 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 llvm#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. Those changes are also mentioned in Release Notes. --------- Co-authored-by: Vladislav Aranov <[email protected]> Co-authored-by: EugeneZelenko <[email protected]>
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…cker (llvm#122841) 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 llvm#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. Those changes are also mentioned in Release Notes. --------- Co-authored-by: Vladislav Aranov <[email protected]> Co-authored-by: EugeneZelenko <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #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.
Those changes are also mentioned in Release Notes.
Previous discussion was here: #121960. Had to create a new pull request as previous one got closed accidentally.