Index: clang-tidy/readability/SimplifyBooleanExprCheck.h =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.h +++ clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -30,15 +30,25 @@ /// `e ? false : true` becomes `!e` /// `if (true) t(); else f();` becomes `t();` /// `if (false) t(); else f();` becomes `f();` -/// `if (e) return true; else return false;` becomes `return (e);` -/// `if (e) return false; else return true;` becomes `return !(e);` +/// `if (e) return true; else return false;` becomes `return e;` +/// `if (e) return false; else return true;` becomes `return !e;` /// `if (e) b = true; else b = false;` becomes `b = e;` -/// `if (e) b = false; else b = true;` becomes `b = !(e);` +/// `if (e) b = false; else b = true;` becomes `b = !e;` +/// +/// Parenthesis from the resulting expression `e` are removed whenever +/// possible. +/// +/// 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. /// class SimplifyBooleanExprCheck : public ClangTidyCheck { public: - SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context); + + void storeOptions(ClangTidyOptions::OptionMap &Options) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; @@ -92,6 +102,9 @@ void replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated = false); + + const bool ChainedConditionalReturn; + const bool ChainedConditionalAssignment; }; } // namespace readability Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -108,20 +108,25 @@ StringRef NegatedOperator = negatedOperator(BinOp); if (!NegatedOperator.empty()) { return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator + - " " + getText(Result, *BinOp->getRHS())) - .str(); + " " + getText(Result, *BinOp->getRHS())).str(); } } } StringRef Text = getText(Result, *E); return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")" : "!" + Text) - : Text) - .str(); + : Text).str(); } } // namespace +SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)), + ChainedConditionalAssignment( + Options.get("ChainedConditionalAssignment", 0U)) {} + void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder, bool Value, StringRef OperatorName, @@ -199,10 +204,18 @@ void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder, bool Value, StringRef Id) { - Finder->addMatcher(ifStmt(isExpansionInMainFile(), - hasThen(ReturnsBool(Value, ThenLiteralId)), - hasElse(ReturnsBool(!Value))).bind(Id), - this); + if (ChainedConditionalReturn) { + Finder->addMatcher(ifStmt(isExpansionInMainFile(), + hasThen(ReturnsBool(Value, ThenLiteralId)), + hasElse(ReturnsBool(!Value))).bind(Id), + this); + } else { + Finder->addMatcher(ifStmt(isExpansionInMainFile(), + unless(hasParent(ifStmt())), + hasThen(ReturnsBool(Value, ThenLiteralId)), + hasElse(ReturnsBool(!Value))).bind(Id), + this); + } } void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder, @@ -220,9 +233,22 @@ hasRHS(boolLiteral(equals(!Value)))); auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1), hasAnySubstatement(SimpleElse))); - Finder->addMatcher( - ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id), - this); + if (ChainedConditionalAssignment) { + Finder->addMatcher( + ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id), + this); + } else { + Finder->addMatcher(ifStmt(isExpansionInMainFile(), + unless(hasParent(ifStmt())), hasThen(Then), + hasElse(Else)).bind(Id), + this); + } +} + +void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn); + Options.store(Opts, "ChainedConditionalAssignment", + ChainedConditionalAssignment); } void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { Index: test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp @@ -0,0 +1,35 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: 1}]}" -- +// REQUIRES: shell + +void chained_conditional_compound_assignment(int i) { + bool b; + if (i < 0) { + b = true; + } else if (i < 10) { + b = false; + } else if (i > 20) { + b = true; + } else { + b = false; + } + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr] + // CHECK-FIXES: {{^}} } else if (i < 10) {{{$}} + // CHECK-FIXES-NEXT: {{^}} b = false;{{$}} + // CHECK-FIXES-NEXT: {{^}} } else b = i > 20;{{$}} +} + +void chained_conditional_assignment(int i) { + bool b; + if (i < 0) + b = true; + else if (i < 10) + b = false; + else if (i > 20) + b = true; + else + b = false; + // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: {{.*}} in conditional assignment + // CHECK-FIXES: {{^}} else if (i < 10) + // CHECK-FIXES-NEXT: {{^}} b = false; + // CHECK-FIXES-NEXT: {{^}} else b = i > 20; +} Index: test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp @@ -0,0 +1,33 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: 1}]}" -- +// REQUIRES: shell + +bool chained_conditional_compound_return(int i) { + if (i < 0) { + return true; + } else if (i < 10) { + return false; + } else if (i > 20) { + return true; + } else { + return false; + } + // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr] + // CHECK-FIXES: {{^}} } else if (i < 10) {{{$}} + // CHECK-FIXES-NEXT: {{^}} return false;{{$}} + // CHECK-FIXES-NEXT: {{^}} } else return i > 20;{{$}} +} + +bool chained_conditional_return(int i) { + if (i < 0) + return true; + else if (i < 10) + return false; + else if (i > 20) + return true; + else + return false; + // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement + // CHECK-FIXES: {{^}} else if (i < 10) + // CHECK-FIXES-NEXT: {{^}} return false; + // CHECK-FIXES-NEXT: {{^}} else return i > 20; +} 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 @@ -541,3 +541,55 @@ } else f = false; } + +// unchanged: chained return statements, but ChainedConditionalReturn not set +bool chained_conditional_compound_return(int i) { + if (i < 0) { + return true; + } else if (i < 10) { + return false; + } else if (i > 20) { + return true; + } else { + return false; + } +} + +// unchanged: chained return statements, but ChainedConditionalReturn not set +bool chained_conditional_return(int i) { + if (i < 0) + return true; + else if (i < 10) + return false; + else if (i > 20) + return true; + else + return false; +} + +// unchanged: chained assignments, but ChainedConditionalAssignment not set +void chained_conditional_compound_assignment(int i) { + bool b; + if (i < 0) { + b = true; + } else if (i < 10) { + b = false; + } else if (i > 20) { + b = true; + } else { + b = false; + } +} + +// unchanged: chained return statements, but ChainedConditionalReturn not set +void chained_conditional_assignment(int i) { + bool b; + if (i < 0) + b = true; + else if (i < 10) + b = false; + else if (i > 20) + b = true; + else + b = false; +}