Author: alexfh Date: Sun May 17 07:31:12 2015 New Revision: 237541 URL: http://llvm.org/viewvc/llvm-project?rev=237541&view=rev Log: [clang-tidy] Enhance clang-tidy readability-simplify-boolean-expr check...
Enhance clang-tidy readability-simplify-boolean-expr check to handle chained conditional assignment and chained conditional return. Based on feedback from applying this tool to the clang/LLVM codebase, this changeset improves the readability-simplify-boolean-expr check so that conditional assignment or return statements at the end of a chain of if/else if statements are left unchanged unless a configuration option is supplied. http://reviews.llvm.org/D8996 Patch by Richard Thomson! Added: clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp Modified: clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.h clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr.cpp Modified: clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp?rev=237541&r1=237540&r2=237541&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp Sun May 17 07:31:12 2015 @@ -108,20 +108,25 @@ std::string replacementExpression(const 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::matchTern 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 @@ void SimplifyBooleanExprCheck::matchIfAs 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) { Modified: clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.h?rev=237541&r1=237540&r2=237541&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.h Sun May 17 07:31:12 2015 @@ -30,15 +30,25 @@ namespace readability { /// `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 @@ private: void replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated = false); + + const bool ChainedConditionalReturn; + const bool ChainedConditionalAssignment; }; } // namespace readability Added: clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp?rev=237541&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp (added) +++ clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp Sun May 17 07:31:12 2015 @@ -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; +} Added: clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp?rev=237541&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp (added) +++ clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp Sun May 17 07:31:12 2015 @@ -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; +} Modified: clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr.cpp?rev=237541&r1=237540&r2=237541&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/readability-simplify-bool-expr.cpp Sun May 17 07:31:12 2015 @@ -541,3 +541,55 @@ void complex_conditional_assignment_stat } 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; +} _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
