https://github.com/earnol updated https://github.com/llvm/llvm-project/pull/121960
>From 78272d21685abbf50921ffeff1ae5ca5c34456b1 Mon Sep 17 00:00:00 2001 From: Vladislav Aranov <vladislav.ara...@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 | 117 +++++++++++--- .../checkers/misc/redundant-expression.cpp | 150 ++++++++++++++++++ 2 files changed, 247 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp index fc35bc22c52e04..2419915d40e156 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) { + 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)) @@ -852,6 +874,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 Expr *Lhs = BinOp->getLHS(); + const Expr *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 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) { + 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 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 +1163,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 +1207,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 +1349,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/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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
