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, @@ -518,6 +519,9 @@ if (!OverloadedFunctionDecl) return false; + if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false)) + return false; + if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr( Value, *Result.Context)) return false; @@ -558,7 +562,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, @@ -683,6 +687,33 @@ .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 @@ -859,20 +890,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; } } } @@ -986,6 +1016,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 +1064,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