Author: earnol Date: 2025-02-10T13:16:37-05:00 New Revision: 1e14edb8981326e18509409be5c95e0c8c740891
URL: https://github.com/llvm/llvm-project/commit/1e14edb8981326e18509409be5c95e0c8c740891 DIFF: https://github.com/llvm/llvm-project/commit/1e14edb8981326e18509409be5c95e0c8c740891.diff LOG: [clang-tidy] Address false positives in misc-redundant-expression checker (#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 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. Those changes are also mentioned in Release Notes. --------- Co-authored-by: Vladislav Aranov <vladislav.ara...@ericsson.com> Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> Added: Modified: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/misc/redundant-expression.rst clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp index fc35bc22c52e042..c249244b1a1b27f 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,83 @@ 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; + + // Do running comparison ignoring spaces + llvm::StringRef L = Left.trim(); + llvm::StringRef R = Right.trim(); + while (!L.empty() && !R.empty()) { + L = L.ltrim(); + R = R.ltrim(); + if (L.empty() && R.empty()) + return true; + // If symbol compared are diff erent ==> strings are not the same + if (L.front() != R.front()) + return false; + L = L.drop_front(); + R = R.drop_front(); + } + return L.empty() && R.empty(); +} + +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, Context->getLangOpts(), 0); + const llvm::StringRef R = Lexer::getSourceText( + CharSourceRange::getTokenRange(Rsr), SM, Context->getLangOpts(), 0); + return areStringsSameIgnoreSpaces(L, R); + } + // Left is macro but right is not so they are not same macro or literal + return false; + } + 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, + Context->getLangOpts(), 0); + const llvm::StringRef R = Lexer::getSourceText( + CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM, + Context->getLangOpts(), 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 +1183,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 +1227,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 +1369,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/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3bddeeda06e06b9..8353138250a2c05 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,10 @@ Changes in existing checks <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying additional C++ member functions to match. +- Improved :doc:`misc-redundant-expression + <clang-tidy/checks/misc/redundant-expression>` check by providing additional + examples and fixing some macro related false positives. + Removed checks ^^^^^^^^^^^^^^ 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 44f0772d0bedf90..cea998d39bd73b0 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 diff er 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 7396d2dce76c43c..95d8ecb15a4f1e7 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,223 @@ 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 + + // Test prefixes + if (+VAL_6 == +VAL_6) return 2; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent + if (-VAL_6 == -VAL_6) return 2; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent + if ((+VAL_6) == (+VAL_6)) return 2; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: both sides of operator are equivalent + if ((-VAL_6) == (-VAL_6)) return 2; + // CHECK-MESSAGES: :[[@LINE-1]]:16: 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; + + // Negative cases + 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); + + // Positive cases + ret += (+getValue(1)) > (+getValue( 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: both sides of operator are equivalent + ret += (-getValue(1)) > (-getValue( 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: both sides of operator are equivalent + + ret += +getValue(1) > +getValue( 1); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both sides of operator are equivalent + ret += -getValue(1) > -getValue( 1); + // CHECK-MESSAGES: :[[@LINE-1]]:23: 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]]: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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
