Index: clang-tidy/readability/SimplifyBooleanExprCheck.h =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.h +++ clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -19,75 +19,8 @@ /// Looks for boolean expressions involving boolean constants and simplifies /// them to use the appropriate boolean expression directly. /// -/// Examples: -/// -/// =========================================== ================ -/// Initial expression Result -/// ------------------------------------------- ---------------- -/// `if (b == true)` `if (b)` -/// `if (b == false)` `if (!b)` -/// `if (b && true)` `if (b)` -/// `if (b && false)` `if (false)` -/// `if (b || true)` `if (true)` -/// `if (b || false)` `if (b)` -/// `e ? true : false` `e` -/// `e ? false : true` `!e` -/// `if (true) t(); else f();` `t();` -/// `if (false) t(); else f();` `f();` -/// `if (e) return true; else return false;` `return e;` -/// `if (e) return false; else return true;` `return !e;` -/// `if (e) b = true; else b = false;` `b = e;` -/// `if (e) b = false; else b = true;` `b = !e;` -/// `if (e) return true; return false;` `return e;` -/// `if (e) return false; return true;` `return !e;` -/// =========================================== ================ -/// -/// The resulting expression `e` is modified as follows: -/// 1. Unnecessary parentheses around the expression are removed. -/// 2. Negated applications of `!` are eliminated. -/// 3. Negated applications of comparison operators are changed to use the -/// opposite condition. -/// 4. Implicit conversions of pointer to `bool` are replaced with explicit -/// comparisons to `nullptr`. -/// 5. Implicit casts to `bool` are replaced with explicit casts to `bool`. -/// 6. Object expressions with `explicit operator bool` conversion operators -/// are replaced with explicit casts to `bool`. -/// -/// Examples: -/// 1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant -/// parentheses and becomes `bool b = i < 0;`. -/// -/// 2. The conditional return `if (!b) return false; return true;` has an -/// implied double negation and becomes `return b;`. -/// -/// 3. The conditional return `if (i < 0) return false; return true;` becomes -/// `return i >= 0;`. -/// -/// The conditional return `if (i != 0) return false; return true;` becomes -/// `return i == 0;`. -/// -/// 4. The conditional return `if (p) return true; return false;` has an -/// implicit conversion of a pointer to `bool` and becomes -/// `return p != nullptr;`. -/// -/// The ternary assignment `bool b = (i & 1) ? true : false;` has an -/// implicit conversion of `i & 1` to `bool` and becomes -/// `bool b = static_cast(i & 1);`. -/// -/// 5. The conditional return `if (i & 1) return true; else return false;` has -/// an implicit conversion of an integer quantity `i & 1` to `bool` and -/// becomes `return static_cast(i & 1);` -/// -/// 6. Given `struct X { explicit operator bool(); };`, and an instance `x` of -/// `struct X`, the conditional return `if (x) return true; return false;` -/// becomes `return static_cast(x);` -/// -/// When a conditional boolean return or assignment appears at the end of a -/// chain of `if`, `else if` statements, the conditional statement is left -/// unchanged unless the option `ChainedConditionalReturn` or -/// `ChainedConditionalAssignment`, respectively, is specified as non-zero. -/// The default value for both options is zero. -/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html class SimplifyBooleanExprCheck : public ClangTidyCheck { public: SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context); Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -85,10 +85,11 @@ E = E->IgnoreImpCasts(); if (isa(E) || isa(E)) return true; - if (const auto *Op = dyn_cast(E)) { + + if (const auto *Op = dyn_cast(E)) return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call && Op->getOperator() != OO_Subscript; - } + return false; } @@ -107,12 +108,8 @@ } std::pair OperatorNames[] = { - {OO_EqualEqual, "=="}, - {OO_ExclaimEqual, "!="}, - {OO_Less, "<"}, - {OO_GreaterEqual, ">="}, - {OO_Greater, ">"}, - {OO_LessEqual, "<="}}; + {OO_EqualEqual, "=="}, {OO_ExclaimEqual, "!="}, {OO_Less, "<"}, + {OO_GreaterEqual, ">="}, {OO_Greater, ">"}, {OO_LessEqual, "<="}}; StringRef getOperatorName(OverloadedOperatorKind OpKind) { for (auto Name : OperatorNames) { @@ -148,7 +145,15 @@ bool needsNullPtrComparison(const Expr *E) { if (const auto *ImpCast = dyn_cast(E)) - return ImpCast->getCastKind() == CK_PointerToBoolean; + return ImpCast->getCastKind() == CK_PointerToBoolean || + ImpCast->getCastKind() == CK_MemberPointerToBoolean; + + return false; +} + +bool needsZeroComparison(const Expr *E) { + if (const auto *ImpCast = dyn_cast(E)) + return ImpCast->getCastKind() == CK_IntegralToBoolean; return false; } @@ -172,6 +177,29 @@ return !E->getType()->isBooleanType(); } +std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result, + const Expr *E, bool Negated, + const char *Constant) { + E = E->IgnoreImpCasts(); + const std::string ExprText = + (isa(E) ? ("(" + getText(Result, *E) + ")") + : getText(Result, *E)) + .str(); + return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant; +} + +std::string compareExpressionToNullPtr(const MatchFinder::MatchResult &Result, + const Expr *E, bool Negated) { + const char *NullPtr = + Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL"; + return compareExpressionToConstant(Result, E, Negated, NullPtr); +} + +std::string compareExpressionToZero(const MatchFinder::MatchResult &Result, + const Expr *E, bool Negated) { + return compareExpressionToConstant(Result, E, Negated, "0"); +} + std::string replacementExpression(const MatchFinder::MatchResult &Result, bool Negated, const Expr *E) { E = E->ignoreParenBaseCasts(); @@ -180,14 +208,20 @@ if (const auto *UnOp = dyn_cast(E)) { if (UnOp->getOpcode() == UO_LNot) { if (needsNullPtrComparison(UnOp->getSubExpr())) - return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str(); + return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), true); + + if (needsZeroComparison(UnOp->getSubExpr())) + return compareExpressionToZero(Result, UnOp->getSubExpr(), true); return replacementExpression(Result, false, UnOp->getSubExpr()); } } if (needsNullPtrComparison(E)) - return (getText(Result, *E) + " == nullptr").str(); + return compareExpressionToNullPtr(Result, E, false); + + if (needsZeroComparison(E)) + return compareExpressionToZero(Result, E, false); StringRef NegatedOperator; const Expr *LHS = nullptr; @@ -203,18 +237,21 @@ RHS = OpExpr->getArg(1); } } - if (!NegatedOperator.empty() && LHS && RHS) { + if (!NegatedOperator.empty() && LHS && RHS) return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " + - getText(Result, *RHS)).str(), + getText(Result, *RHS)) + .str(), NeedsStaticCast)); - } StringRef Text = getText(Result, *E); if (!NeedsStaticCast && needsParensAfterUnaryNegation(E)) return ("!(" + Text + ")").str(); if (needsNullPtrComparison(E)) - return (getText(Result, *E) + " == nullptr").str(); + return compareExpressionToNullPtr(Result, E, false); + + if (needsZeroComparison(E)) + return compareExpressionToZero(Result, E, false); return ("!" + asBool(Text, NeedsStaticCast)); } @@ -222,12 +259,18 @@ if (const auto *UnOp = dyn_cast(E)) { if (UnOp->getOpcode() == UO_LNot) { if (needsNullPtrComparison(UnOp->getSubExpr())) - return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str(); + return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), false); + + if (needsZeroComparison(UnOp->getSubExpr())) + return compareExpressionToZero(Result, UnOp->getSubExpr(), false); } } if (needsNullPtrComparison(E)) - return (getText(Result, *E) + " != nullptr").str(); + return compareExpressionToNullPtr(Result, E, true); + + if (needsZeroComparison(E)) + return compareExpressionToZero(Result, E, true); return asBool(getText(Result, *E), NeedsStaticCast); } @@ -258,6 +301,26 @@ return nullptr; } +bool containsDiscardedTokens(const MatchFinder::MatchResult &Result, + CharSourceRange CharRange) { + std::string ReplacementText = + Lexer::getSourceText(CharRange, *Result.SourceManager, + Result.Context->getLangOpts()) + .str(); + Lexer Lex(CharRange.getBegin(), Result.Context->getLangOpts(), + ReplacementText.data(), ReplacementText.data(), + ReplacementText.data() + ReplacementText.size()); + Lex.SetCommentRetentionState(true); + + Token Tok; + while (!Lex.LexFromRawLexer(Tok)) { + if (Tok.is(tok::TokenKind::comment) || Tok.is(tok::TokenKind::hash)) + return true; + } + + return false; +} + } // namespace SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name, @@ -301,12 +364,13 @@ StringRef OperatorName, StringRef BooleanId) { Finder->addMatcher( - binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName), - hasLHS(allOf(expr().bind(LHSId), - ignoringImpCasts(cxxBoolLiteral(equals(Value)) - .bind(BooleanId)))), - hasRHS(expr().bind(RHSId)), - unless(hasRHS(hasDescendant(cxxBoolLiteral())))), + binaryOperator( + isExpansionInMainFile(), hasOperatorName(OperatorName), + hasLHS(allOf( + expr().bind(LHSId), + ignoringImpCasts(cxxBoolLiteral(equals(Value)).bind(BooleanId)))), + hasRHS(expr().bind(RHSId)), + unless(hasRHS(hasDescendant(cxxBoolLiteral())))), this); } @@ -315,22 +379,24 @@ StringRef OperatorName, StringRef BooleanId) { Finder->addMatcher( - binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName), - unless(hasLHS(hasDescendant(cxxBoolLiteral()))), - hasLHS(expr().bind(LHSId)), - hasRHS(allOf(expr().bind(RHSId), - ignoringImpCasts(cxxBoolLiteral(equals(Value)) - .bind(BooleanId))))), + binaryOperator( + isExpansionInMainFile(), hasOperatorName(OperatorName), + unless(hasLHS(hasDescendant(cxxBoolLiteral()))), + hasLHS(expr().bind(LHSId)), + hasRHS(allOf(expr().bind(RHSId), + ignoringImpCasts( + cxxBoolLiteral(equals(Value)).bind(BooleanId))))), this); } void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder, bool Value, StringRef BooleanId) { - Finder->addMatcher(ifStmt(isExpansionInMainFile(), - hasCondition(cxxBoolLiteral(equals(Value)) - .bind(BooleanId))).bind(IfStmtId), - this); + Finder->addMatcher( + ifStmt(isExpansionInMainFile(), + hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId))) + .bind(IfStmtId), + this); } void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder, @@ -346,18 +412,19 @@ void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder, bool Value, StringRef Id) { - if (ChainedConditionalReturn) { + if (ChainedConditionalReturn) Finder->addMatcher(ifStmt(isExpansionInMainFile(), hasThen(returnsBool(Value, ThenLiteralId)), - hasElse(returnsBool(!Value))).bind(Id), + hasElse(returnsBool(!Value))) + .bind(Id), this); - } else { + else Finder->addMatcher(ifStmt(isExpansionInMainFile(), unless(hasParent(ifStmt())), hasThen(returnsBool(Value, ThenLiteralId)), - hasElse(returnsBool(!Value))).bind(Id), + hasElse(returnsBool(!Value))) + .bind(Id), this); - } } void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder, @@ -375,16 +442,16 @@ hasRHS(cxxBoolLiteral(equals(!Value)))); auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1), hasAnySubstatement(SimpleElse))); - if (ChainedConditionalAssignment) { + if (ChainedConditionalAssignment) Finder->addMatcher( ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id), this); - } else { + else Finder->addMatcher(ifStmt(isExpansionInMainFile(), unless(hasParent(ifStmt())), hasThen(Then), - hasElse(Else)).bind(Id), + hasElse(Else)) + .bind(Id), this); - } } void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder, @@ -395,7 +462,8 @@ unless(hasElse(stmt())))), hasAnySubstatement( returnStmt(has(cxxBoolLiteral(equals(!Value)))) - .bind(CompoundReturnId)))).bind(Id), + .bind(CompoundReturnId)))) + .bind(Id), this); } @@ -444,69 +512,46 @@ void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) { if (const CXXBoolLiteralExpr *LeftRemoved = - getBoolLiteral(Result, RightExpressionId)) { + getBoolLiteral(Result, RightExpressionId)) replaceWithExpression(Result, LeftRemoved, false); - } else if (const CXXBoolLiteralExpr *RightRemoved = - getBoolLiteral(Result, LeftExpressionId)) { + else if (const CXXBoolLiteralExpr *RightRemoved = + getBoolLiteral(Result, LeftExpressionId)) replaceWithExpression(Result, RightRemoved, true); - } else if (const CXXBoolLiteralExpr *NegatedLeftRemoved = - getBoolLiteral(Result, NegatedRightExpressionId)) { + else if (const CXXBoolLiteralExpr *NegatedLeftRemoved = + getBoolLiteral(Result, NegatedRightExpressionId)) replaceWithExpression(Result, NegatedLeftRemoved, false, true); - } else if (const CXXBoolLiteralExpr *NegatedRightRemoved = - getBoolLiteral(Result, NegatedLeftExpressionId)) { + else if (const CXXBoolLiteralExpr *NegatedRightRemoved = + getBoolLiteral(Result, NegatedLeftExpressionId)) replaceWithExpression(Result, NegatedRightRemoved, true, true); - } else if (const CXXBoolLiteralExpr *TrueConditionRemoved = - getBoolLiteral(Result, ConditionThenStmtId)) { + else if (const CXXBoolLiteralExpr *TrueConditionRemoved = + getBoolLiteral(Result, ConditionThenStmtId)) replaceWithThenStatement(Result, TrueConditionRemoved); - } else if (const CXXBoolLiteralExpr *FalseConditionRemoved = - getBoolLiteral(Result, ConditionElseStmtId)) { + else if (const CXXBoolLiteralExpr *FalseConditionRemoved = + getBoolLiteral(Result, ConditionElseStmtId)) replaceWithElseStatement(Result, FalseConditionRemoved); - } else if (const auto *Ternary = - Result.Nodes.getNodeAs(TernaryId)) { + else if (const auto *Ternary = + Result.Nodes.getNodeAs(TernaryId)) replaceWithCondition(Result, Ternary); - } else if (const auto *TernaryNegated = - Result.Nodes.getNodeAs( - TernaryNegatedId)) { + else if (const auto *TernaryNegated = + Result.Nodes.getNodeAs(TernaryNegatedId)) replaceWithCondition(Result, TernaryNegated, true); - } else if (const auto *If = Result.Nodes.getNodeAs(IfReturnsBoolId)) { + else if (const auto *If = Result.Nodes.getNodeAs(IfReturnsBoolId)) replaceWithReturnCondition(Result, If); - } else if (const auto *IfNot = - Result.Nodes.getNodeAs(IfReturnsNotBoolId)) { + else if (const auto *IfNot = + Result.Nodes.getNodeAs(IfReturnsNotBoolId)) replaceWithReturnCondition(Result, IfNot, true); - } else if (const auto *IfAssign = - Result.Nodes.getNodeAs(IfAssignBoolId)) { + else if (const auto *IfAssign = + Result.Nodes.getNodeAs(IfAssignBoolId)) replaceWithAssignment(Result, IfAssign); - } else if (const auto *IfAssignNot = - Result.Nodes.getNodeAs(IfAssignNotBoolId)) { + else if (const auto *IfAssignNot = + Result.Nodes.getNodeAs(IfAssignNotBoolId)) replaceWithAssignment(Result, IfAssignNot, true); - } else if (const auto *Compound = - Result.Nodes.getNodeAs(CompoundBoolId)) { + else if (const auto *Compound = + Result.Nodes.getNodeAs(CompoundBoolId)) replaceCompoundReturnWithCondition(Result, Compound); - } else if (const auto *Compound = - Result.Nodes.getNodeAs(CompoundNotBoolId)) { + else if (const auto *Compound = + Result.Nodes.getNodeAs(CompoundNotBoolId)) replaceCompoundReturnWithCondition(Result, Compound, true); - } -} - -bool containsDiscardedTokens( - const ast_matchers::MatchFinder::MatchResult &Result, - CharSourceRange CharRange) { - std::string ReplacementText = - Lexer::getSourceText(CharRange, *Result.SourceManager, - Result.Context->getLangOpts()) - .str(); - Lexer Lex(CharRange.getBegin(), Result.Context->getLangOpts(), - ReplacementText.data(), ReplacementText.data(), - ReplacementText.data() + ReplacementText.size()); - Lex.SetCommentRetentionState(true); - Token Tok; - - while (!Lex.LexFromRawLexer(Tok)) { - if (Tok.is(tok::TokenKind::comment) || Tok.is(tok::TokenKind::hash)) - return true; - } - - return false; } void SimplifyBooleanExprCheck::issueDiag( @@ -582,7 +627,7 @@ // The body shouldn't be empty because the matcher ensures that it must // contain at least two statements: // 1) A `return` statement returning a boolean literal `false` or `true` - // 2) An `if` statement with no `else` clause that consists fo a single + // 2) An `if` statement with no `else` clause that consists of a single // `return` statement returning the opposite boolean literal `true` or // `false`. assert(Compound->size() >= 2); Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst =================================================================== --- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst +++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst @@ -35,11 +35,14 @@ 2. Negated applications of ``!`` are eliminated. 3. Negated applications of comparison operators are changed to use the opposite condition. - 4. Implicit conversions of pointer to ``bool`` are replaced with explicit - comparisons to ``nullptr``. + 4. Implicit conversions of pointers, including pointers to members, to + ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11 + or ``NULL`` in C++98/03. 5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``. 6. Object expressions with ``explicit operator bool`` conversion operators are replaced with explicit casts to ``bool``. + 7. Implicit conversions of integral types to ``bool`` are replaced with + explicit comparisons to ``0``. Examples: 1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant @@ -60,11 +63,11 @@ The ternary assignment ``bool b = (i & 1) ? true : false;`` has an implicit conversion of ``i & 1`` to ``bool`` and becomes - ``bool b = static_cast(i & 1);``. + ``bool b = (i & 1) != 0;``. 5. The conditional return ``if (i & 1) return true; else return false;`` has an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and - becomes ``return static_cast(i & 1);`` + becomes ``return (i & 1) != 0;`` 6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of ``struct X``, the conditional return ``if (x) return true; return false;`` Index: test/clang-tidy/readability-simplify-bool-expr.cpp =================================================================== --- test/clang-tidy/readability-simplify-bool-expr.cpp +++ test/clang-tidy/readability-simplify-bool-expr.cpp @@ -690,7 +690,7 @@ } } // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return -// CHECK-FIXES: {{^}} return static_cast(i & 1);{{$}} +// CHECK-FIXES: {{^}} return (i & 1) != 0;{{$}} bool negated_if_implicit_bool_expr(int i) { if (i - 1) { @@ -700,7 +700,7 @@ } } // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return -// CHECK-FIXES: {{^}} return !static_cast(i - 1);{{$}} +// CHECK-FIXES: {{^}} return (i - 1) == 0;{{$}} bool implicit_int(int i) { if (i) { @@ -710,7 +710,7 @@ } } // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return -// CHECK-FIXES: {{^}} return static_cast(i);{{$}} +// CHECK-FIXES: {{^}} return i != 0;{{$}} bool explicit_bool(bool b) { if (b) { @@ -757,7 +757,7 @@ } } // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return -// CHECK-FIXES: {{^}} return static_cast(~i);{{$}} +// CHECK-FIXES: {{^}} return ~i != 0;{{$}} bool logical_or(bool a, bool b) { if (a || b) { @@ -830,7 +830,7 @@ bool b = i ? true : false; } // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result -// CHECK-FIXES: bool b = static_cast(i);{{$}} +// CHECK-FIXES: bool b = i != 0;{{$}} bool non_null_pointer_condition(int *p1) { if (p1) { @@ -895,3 +895,38 @@ // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return // CHECK-FIXES: {{^}} if (b) { // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false + +bool integer_not_zero(int i) { + if (i) { + return false; + } else { + return true; + } +} +// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return +// CHECK-FIXES: {{^}} return i == 0;{{$}} + +class A { +public: + int m; +}; + +bool member_pointer_nullptr(int A::*p) { + if (p) { + return true; + } else { + return false; + } +} +// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return +// CHECK-FIXES: return p != nullptr;{{$}} + +bool integer_member_implicit_cast(A *p) { + if (p->m) { + return true; + } else { + return false; + } +} +// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return +// CHECK-FIXES: return p->m != 0;{{$}}