https://github.com/earnol updated https://github.com/llvm/llvm-project/pull/122841
>From 1e0f301f1c41e7f678c3eac2a067a7c81caa3228 Mon Sep 17 00:00:00 2001 From: Vladislav Aranov <vladislav.ara...@ericsson.com> Date: Mon, 13 Jan 2025 20:19:58 -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 | 166 ++++++++++++--- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- .../checkers/misc/redundant-expression.cpp | 193 ++++++++++++++++++ clang/docs/ReleaseNotes.rst | 3 + 4 files changed, 339 insertions(+), 25 deletions(-) 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/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3fe2f0ce01bccd..cf20e079936bef 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -290,7 +290,7 @@ Changes in existing checks - Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc/redundant-expression>` check by extending the checker to detect floating point and integer literals in redundant - expressions. + expressions. Some macro related false positives had been removed. - Improved :doc:`misc-unconventional-assign-operator <clang-tidy/checks/misc/unconventional-assign-operator>` check to avoid 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 ^^^^^^^^^^^^^^ _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
