Index: clang-tidy/misc/RedundantExpressionCheck.cpp =================================================================== --- clang-tidy/misc/RedundantExpressionCheck.cpp +++ clang-tidy/misc/RedundantExpressionCheck.cpp @@ -22,6 +22,7 @@ #include "llvm/Support/Casting.h" #include #include +#include #include #include #include @@ -198,7 +199,7 @@ } // Returns whether the ranges covered by the union of both relational -// expressions covers the whole domain (i.e. x < 10 and x > 0). +// expressions cover the whole domain (i.e. x < 10 and x > 0). static bool rangesFullyCoverDomain(BinaryOperatorKind OpcodeLHS, const APSInt &ValueLHS, BinaryOperatorKind OpcodeRHS, @@ -514,17 +515,22 @@ Value = APSInt(32, false); } else if (const auto *OverloadedOperatorExpr = Result.Nodes.getNodeAs(OverloadId)) { - const auto *OverloadedFunctionDecl = dyn_cast_or_null(OverloadedOperatorExpr->getCalleeDecl()); + const auto *OverloadedFunctionDecl = + dyn_cast_or_null(OverloadedOperatorExpr->getCalleeDecl()); if (!OverloadedFunctionDecl) return false; + if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false)) + return false; + if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr( Value, *Result.Context)) return false; Symbol = OverloadedOperatorExpr->getArg(0); OperandExpr = OverloadedOperatorExpr; - Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator()); + Opcode = BinaryOperator::getOverloadedOpcode( + OverloadedOperatorExpr->getOperator()); return BinaryOperator::isComparisonOp(Opcode); } else { @@ -542,7 +548,8 @@ } // 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(BinOp->getLHS()); const auto *RhsBinOp = dyn_cast(BinOp->getRHS()); @@ -558,7 +565,7 @@ } // Retrieves integer constant subexpressions from binary operator expressions -// that have two equivalent sides +// that have two equivalent sides. // E.g.: from (X == 5) && (X == 5) retrieves 5 and 5. static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp, BinaryOperatorKind &MainOpcode, @@ -608,7 +615,7 @@ const LangOptions &LO = AstCtx->getLangOpts(); return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) == - Lexer::getImmediateMacroName(RhsLoc, SM, LO)); + Lexer::getImmediateMacroName(RhsLoc, SM, LO)); } static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, @@ -676,13 +683,41 @@ // Match ineffective bitwise operator expressions like: x & 0 or x |= ~0. Finder->addMatcher( - binaryOperator(anyOf(hasOperatorName("|"), hasOperatorName("&"), - hasOperatorName("|="), hasOperatorName("&=")), - hasEitherOperand(matchIntegerConstantExpr("ineffective-bitwise")), - hasEitherOperand(matchSymbolicExpr("ineffective-bitwise"))) + binaryOperator( + anyOf(hasOperatorName("|"), hasOperatorName("&"), + hasOperatorName("|="), hasOperatorName("&=")), + hasEitherOperand(matchIntegerConstantExpr("ineffective-bitwise")), + hasEitherOperand(matchSymbolicExpr("ineffective-bitwise"))) .bind("ineffective-bitwise"), this); + // Match expressions like: !(1 | 2 | 3) + Finder->addMatcher( + implicitCastExpr( + hasImplicitDestinationType(isInteger()), + has(unaryOperator( + hasOperatorName("!"), + hasUnaryOperand(ignoringParenImpCasts(binaryOperator( + anyOf(hasOperatorName("|"), hasOperatorName("&")), + hasLHS(anyOf(binaryOperator(anyOf(hasOperatorName("|"), + hasOperatorName("&"))), + integerLiteral())), + hasRHS(integerLiteral()))))) + .bind("logical-bitwise-confusion"))), + this); + + // Match expressions like: (X << 8) & 0xFF + Finder->addMatcher( + binaryOperator(hasOperatorName("&"), + hasEitherOperand(ignoringParenImpCasts(binaryOperator( + hasOperatorName("<<"), + hasRHS(ignoringParenImpCasts( + integerLiteral().bind("shift-const")))))), + hasEitherOperand(ignoringParenImpCasts( + integerLiteral().bind("and-const")))) + .bind("left-right-shift-confusion"), + this); + // Match common expressions and apply more checks to find redundant // sub-expressions. // a) Expr K1 == K2 @@ -845,11 +880,12 @@ const Expr *Sym = nullptr, *ConstExpr = nullptr; if (!retrieveSymbolicExpr(Result, "ineffective-bitwise", Sym) || - !retrieveIntegerConstantExpr(Result, "ineffective-bitwise", Value, ConstExpr)) + !retrieveIntegerConstantExpr(Result, "ineffective-bitwise", Value, + ConstExpr)) return; - if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID()) - return; + if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID()) + return; SourceLocation Loc = IneffectiveOperator->getOperatorLoc(); @@ -859,20 +895,19 @@ } else if (exprEvaluatesToBitwiseNegatedZero(Opcode, Value)) { SourceRange ConstExprRange(ConstExpr->getLocStart(), ConstExpr->getLocEnd()); - std::string ConstExprText = Lexer::getSourceText( + StringRef ConstExprText = Lexer::getSourceText( CharSourceRange::getTokenRange(ConstExprRange), *Result.SourceManager, Result.Context->getLangOpts()); - diag(Loc, - "expression always evaluates to '" + ConstExprText + "'"); + diag(Loc, "expression always evaluates to '%0'") << ConstExprText; } else if (exprEvaluatesToSymbolic(Opcode, Value)) { SourceRange SymExprRange(Sym->getLocStart(), Sym->getLocEnd()); - std::string ExprText = Lexer::getSourceText( + StringRef ExprText = Lexer::getSourceText( CharSourceRange::getTokenRange(SymExprRange), *Result.SourceManager, Result.Context->getLangOpts()); - diag(Loc, "expression always evaluates to '" + ExprText + "'"); + diag(Loc, "expression always evaluates to '%0'") << ExprText; } } } @@ -975,7 +1010,8 @@ } if (const auto *Call = Result.Nodes.getNodeAs("call")) { - const auto *OverloadedFunctionDecl = dyn_cast_or_null(Call->getCalleeDecl()); + const auto *OverloadedFunctionDecl = + dyn_cast_or_null(Call->getCalleeDecl()); if (!OverloadedFunctionDecl) return; @@ -986,6 +1022,45 @@ "both sides of overloaded operator are equivalent"); } + if (const auto *NegateOperator = + Result.Nodes.getNodeAs("logical-bitwise-confusion")) { + SourceLocation OperatorLoc = NegateOperator->getOperatorLoc(); + + auto Diag = + diag(OperatorLoc, + "ineffective logical negation operator used; did you mean '~'?"); + SourceLocation LogicalNotLocation = OperatorLoc.getLocWithOffset(1); + + if (!LogicalNotLocation.isMacroID()) + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(OperatorLoc, LogicalNotLocation), "~"); + } + + if (const auto *BinaryAndExpr = Result.Nodes.getNodeAs( + "left-right-shift-confusion")) { + const auto *ShiftingConst = Result.Nodes.getNodeAs("shift-const"); + assert(ShiftingConst && "Expr* 'ShiftingConst' is nullptr!"); + APSInt ShiftingValue; + + if (!ShiftingConst->isIntegerConstantExpr(ShiftingValue, *Result.Context)) + return; + + const auto *AndConst = Result.Nodes.getNodeAs("and-const"); + assert(AndConst && "Expr* 'AndCont' is nullptr!"); + APSInt AndValue; + if (!AndConst->isIntegerConstantExpr(AndValue, *Result.Context)) + return; + + // If ShiftingConst is shifted left with more bits than the position of the + // leftmost 1 in the bit representation of AndValue, AndConstant is + // ineffective. + if (floor(log2(AndValue.getExtValue())) >= ShiftingValue) + return; + + auto Diag = diag(BinaryAndExpr->getOperatorLoc(), + "ineffective bitwise and operation"); + } + // Check for the following bound expressions: // - "binop-const-compare-to-sym", // - "binop-const-compare-to-binop-const", @@ -995,8 +1070,10 @@ // Check for the following bound expression: // - "binop-const-compare-to-const", + // - "ineffective-bitwise" // Produced message: // -> "logical expression is always false/true" + // -> "expression always evaluates to ..." checkBitwiseExpr(Result); // Check for te following bound expression: Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -144,6 +144,15 @@ - Added alias `hicpp-braces-around-statements `_ +- Added new functionality to `misc-redundant-expression + http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html`_ check + + Finds redundant binary operator expressions where the operators are overloaded, + and ones that contain the same macros twice. + Also checks for assignment expressions that do not change the value of the + assigned variable, and expressions that always evaluate to the same value + because of possible operator confusion. + Improvements to include-fixer ----------------------------- Index: test/clang-tidy/misc-redundant-expression.cpp =================================================================== --- test/clang-tidy/misc-redundant-expression.cpp +++ test/clang-tidy/misc-redundant-expression.cpp @@ -154,6 +154,8 @@ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent if (S >= S) return true; // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent + + return true; } #define LT(x, y) (void)((x) < (y)) @@ -697,3 +699,59 @@ return 0; } + +#define FLAG1 1 +#define FLAG2 2 +#define FLAG3 4 +#define FLAGS (FLAG1 | FLAG2 | FLAG3) +#define NOTFLAGS !(FLAG1 | FLAG2 | FLAG3) +int operatorConfusion(int X, int Y, long Z) +{ + // Ineffective & expressions. + Y = (Y << 8) & 0xff; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: ineffective bitwise and operation + Y = (Y << 12) & 0xfff; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: ineffective bitwise and + Y = (Y << 12) & 0xff; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: ineffective bitwise and + Y = (Y << 8) & 0x77; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: ineffective bitwise and + Y = (Y << 5) & 0x11; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: ineffective bitwise and + + // Tests for unmatched types + Z = (Z << 8) & 0xff; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: ineffective bitwise and operation + Y = (Y << 12) & 0xfffL; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: ineffective bitwise and + Z = (Y << 12) & 0xffLL; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: ineffective bitwise and + Y = (Z << 8L) & 0x77L; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: ineffective bitwise and + + // Effective expressions. Do not check. + Y = (Y << 4) & 0x15; + Y = (Y << 3) & 0x250; + Y = (Y << 9) & 0xF33; + + int K = !(1 | 2 | 4); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: ineffective logical negation operator used; did you mean '~'? + // CHECK-FIXES: {{^}} int K = ~(1 | 2 | 4);{{$}} + K = !(FLAG1 & FLAG2 & FLAG3); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ineffective logical negation operator + // CHECK-FIXES: {{^}} K = ~(FLAG1 & FLAG2 & FLAG3);{{$}} + K = !(3 | 4); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: ineffective logical negation operator + // CHECK-FIXES: {{^}} K = ~(3 | 4);{{$}} + int NotFlags = !FLAGS; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: ineffective logical negation operator + // CHECK-FIXES: {{^}} int NotFlags = ~FLAGS;{{$}} + NotFlags = NOTFLAGS; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: ineffective logical negation operator + return !(1 | 2 | 4); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: ineffective logical negation operator + // CHECK-FIXES: {{^}} return ~(1 | 2 | 4);{{$}} +} +#undef FLAG1 +#undef FLAG2 +#undef FLAG3