From f5e72b0448eb1152cbd25303fbd359e540b6cca9 Mon Sep 17 00:00:00 2001 From: Alexander Kornienko Date: Fri, 10 Apr 2015 19:26:43 +0000 Subject: [PATCH] [clang-tidy] Add readability-simplify-boolean-expr check to clang-tidy This check looks for comparisons between boolean expressions and boolean constants and simplifies them to just use the appropriate boolean expression directly. if (b == true) becomes if (b) if (b == false) becomes if (!b) if (b && true) becomes if (b) if (b && false) becomes if (false) if (b || true) becomes if (true) if (b || false) becomes if (b) e ? true : false becomes e 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) b = true; else b = false; becomes b = e; if (e) b = false; else b = true; becomes b = !(e); http://reviews.llvm.org/D7648 Patch by Richard Thomson! llvm-svn: 234626 --- .../clang-tidy/readability/CMakeLists.txt | 1 + .../readability/ReadabilityTidyModule.cpp | 6 +- .../readability/SimplifyBooleanExprCheck.cpp | 375 ++++++++++++ .../readability/SimplifyBooleanExprCheck.h | 101 ++++ .../readability-simplify-bool-expr.cpp | 543 ++++++++++++++++++ 5 files changed, 1025 insertions(+), 1 deletion(-) create mode 100644 clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/readability-simplify-bool-expr.cpp diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index bfa2a77fd5c6..52486203712f 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyReadabilityModule RedundantStringCStrCheck.cpp RedundantSmartptrGetCheck.cpp ShrinkToFitCheck.cpp + SimplifyBooleanExprCheck.cpp LINK_LIBS clangAST diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index c82062092cc5..bf118f88acef 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -18,6 +18,7 @@ #include "RedundantSmartptrGetCheck.h" #include "RedundantStringCStrCheck.h" #include "ShrinkToFitCheck.h" +#include "SimplifyBooleanExprCheck.h" namespace clang { namespace tidy { @@ -40,7 +41,10 @@ public: "readability-redundant-smartptr-get"); CheckFactories.registerCheck( "readability-redundant-string-cstr"); - CheckFactories.registerCheck("readability-shrink-to-fit"); + CheckFactories.registerCheck( + "readability-shrink-to-fit"); + CheckFactories.registerCheck( + "readability-simplify-boolean-expr"); } }; diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp new file mode 100644 index 000000000000..c9a1182ac597 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -0,0 +1,375 @@ +//===--- SimplifyBooleanExpr.cpp clang-tidy ---------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "SimplifyBooleanExprCheck.h" +#include "clang/Lex/Lexer.h" + +#include +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +namespace { + +StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) { + return Lexer::getSourceText(CharSourceRange::getTokenRange(Range), + *Result.SourceManager, + Result.Context->getLangOpts()); +} + +template +StringRef getText(const MatchFinder::MatchResult &Result, T &Node) { + return getText(Result, Node.getSourceRange()); +} + +const char RightExpressionId[] = "bool-op-expr-yields-expr"; +const char LeftExpressionId[] = "expr-op-bool-yields-expr"; +const char NegatedRightExpressionId[] = "bool-op-expr-yields-not-expr"; +const char NegatedLeftExpressionId[] = "expr-op-bool-yields-not-expr"; +const char ConditionThenStmtId[] = "if-bool-yields-then"; +const char ConditionElseStmtId[] = "if-bool-yields-else"; +const char TernaryId[] = "ternary-bool-yields-condition"; +const char TernaryNegatedId[] = "ternary-bool-yields-not-condition"; +const char IfReturnsBoolId[] = "if-return"; +const char IfReturnsNotBoolId[] = "if-not-return"; +const char ThenLiteralId[] = "then-literal"; +const char IfAssignVariableId[] = "if-assign-lvalue"; +const char IfAssignLocId[] = "if-assign-loc"; +const char IfAssignBoolId[] = "if-assign"; +const char IfAssignNotBoolId[] = "if-assign-not"; +const char IfAssignObjId[] = "if-assign-obj"; + +const char IfStmtId[] = "if"; +const char LHSId[] = "lhs-expr"; +const char RHSId[] = "rhs-expr"; + +const char SimplifyOperatorDiagnostic[] = + "redundant boolean literal supplied to boolean operator"; +const char SimplifyConditionDiagnostic[] = + "redundant boolean literal in if statement condition"; + +const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult &Result, + StringRef Id) { + const auto *Literal = Result.Nodes.getNodeAs(Id); + return (Literal && + Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart())) + ? nullptr + : Literal; +} + +internal::Matcher ReturnsBool(bool Value, StringRef Id = "") { + auto SimpleReturnsBool = returnStmt( + has(boolLiteral(equals(Value)).bind(Id.empty() ? "ignored" : Id))); + return anyOf(SimpleReturnsBool, + compoundStmt(statementCountIs(1), has(SimpleReturnsBool))); +} + +bool needsParensAfterUnaryNegation(const Expr *E) { + if (isa(E) || isa(E)) + return true; + if (const auto *Op = dyn_cast(E)) + return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call && + Op->getOperator() != OO_Subscript; + return false; +} + +std::pair Opposites[] = { + std::make_pair(BO_LT, BO_GE), std::make_pair(BO_GT, BO_LE), + std::make_pair(BO_EQ, BO_NE)}; + +StringRef negatedOperator(const BinaryOperator *BinOp) { + const BinaryOperatorKind Opcode = BinOp->getOpcode(); + for (auto NegatableOp : Opposites) { + if (Opcode == NegatableOp.first) + return BinOp->getOpcodeStr(NegatableOp.second); + if (Opcode == NegatableOp.second) + return BinOp->getOpcodeStr(NegatableOp.first); + } + return StringRef(); +} + +std::string replacementExpression(const MatchFinder::MatchResult &Result, + bool Negated, const Expr *E) { + while (const auto *Parenthesized = dyn_cast(E)) { + E = Parenthesized->getSubExpr(); + } + if (Negated) { + if (const auto *BinOp = dyn_cast(E)) { + StringRef NegatedOperator = negatedOperator(BinOp); + if (!NegatedOperator.empty()) { + return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator + + " " + getText(Result, *BinOp->getRHS())) + .str(); + } + } + } + StringRef Text = getText(Result, *E); + return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")" + : "!" + Text) + : Text) + .str(); +} + +} // namespace + +void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder, + bool Value, + StringRef OperatorName, + StringRef BooleanId) { + Finder->addMatcher( + binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName), + hasLHS(allOf(expr().bind(LHSId), + boolLiteral(equals(Value)).bind(BooleanId))), + hasRHS(expr().bind(RHSId)), + unless(hasRHS(hasDescendant(boolLiteral())))), + this); +} + +void SimplifyBooleanExprCheck::matchExprBinOpBool(MatchFinder *Finder, + bool Value, + StringRef OperatorName, + StringRef BooleanId) { + Finder->addMatcher( + binaryOperator( + isExpansionInMainFile(), hasOperatorName(OperatorName), + hasLHS(expr().bind(LHSId)), + unless(hasLHS(anyOf(boolLiteral(), hasDescendant(boolLiteral())))), + hasRHS(allOf(expr().bind(RHSId), + boolLiteral(equals(Value)).bind(BooleanId)))), + this); +} + +void SimplifyBooleanExprCheck::matchBoolCompOpExpr(MatchFinder *Finder, + bool Value, + StringRef OperatorName, + StringRef BooleanId) { + Finder->addMatcher( + binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName), + hasLHS(allOf(expr().bind(LHSId), + ignoringImpCasts(boolLiteral(equals(Value)) + .bind(BooleanId)))), + hasRHS(expr().bind(RHSId)), + unless(hasRHS(hasDescendant(boolLiteral())))), + this); +} + +void SimplifyBooleanExprCheck::matchExprCompOpBool(MatchFinder *Finder, + bool Value, + StringRef OperatorName, + StringRef BooleanId) { + Finder->addMatcher( + binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName), + unless(hasLHS(hasDescendant(boolLiteral()))), + hasLHS(expr().bind(LHSId)), + hasRHS(allOf(expr().bind(RHSId), + ignoringImpCasts(boolLiteral(equals(Value)) + .bind(BooleanId))))), + this); +} + +void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder, + bool Value, + StringRef BooleanId) { + Finder->addMatcher(ifStmt(isExpansionInMainFile(), + hasCondition(boolLiteral(equals(Value)) + .bind(BooleanId))).bind(IfStmtId), + this); +} + +void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder, + bool Value, + StringRef TernaryId) { + Finder->addMatcher( + conditionalOperator(isExpansionInMainFile(), + hasTrueExpression(boolLiteral(equals(Value))), + hasFalseExpression(boolLiteral(equals(!Value)))) + .bind(TernaryId), + this); +} + +void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder, + bool Value, StringRef Id) { + Finder->addMatcher(ifStmt(isExpansionInMainFile(), + hasThen(ReturnsBool(Value, ThenLiteralId)), + hasElse(ReturnsBool(!Value))).bind(Id), + this); +} + +void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder, + bool Value, StringRef Id) { + auto SimpleThen = binaryOperator( + hasOperatorName("="), + hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId)))), + hasLHS(expr().bind(IfAssignVariableId)), + hasRHS(boolLiteral(equals(Value)).bind(IfAssignLocId))); + auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1), + hasAnySubstatement(SimpleThen))); + auto SimpleElse = binaryOperator( + hasOperatorName("="), + hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId)))), + hasRHS(boolLiteral(equals(!Value)))); + auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1), + hasAnySubstatement(SimpleElse))); + Finder->addMatcher( + ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id), + this); +} + +void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { + matchBoolBinOpExpr(Finder, true, "&&", RightExpressionId); + matchBoolBinOpExpr(Finder, false, "||", RightExpressionId); + matchExprBinOpBool(Finder, false, "&&", RightExpressionId); + matchExprBinOpBool(Finder, true, "||", RightExpressionId); + matchBoolCompOpExpr(Finder, true, "==", RightExpressionId); + matchBoolCompOpExpr(Finder, false, "!=", RightExpressionId); + + matchExprBinOpBool(Finder, true, "&&", LeftExpressionId); + matchExprBinOpBool(Finder, false, "||", LeftExpressionId); + matchBoolBinOpExpr(Finder, false, "&&", LeftExpressionId); + matchBoolBinOpExpr(Finder, true, "||", LeftExpressionId); + matchExprCompOpBool(Finder, true, "==", LeftExpressionId); + matchExprCompOpBool(Finder, false, "!=", LeftExpressionId); + + matchBoolCompOpExpr(Finder, false, "==", NegatedRightExpressionId); + matchBoolCompOpExpr(Finder, true, "!=", NegatedRightExpressionId); + + matchExprCompOpBool(Finder, false, "==", NegatedLeftExpressionId); + matchExprCompOpBool(Finder, true, "!=", NegatedLeftExpressionId); + + matchBoolCondition(Finder, true, ConditionThenStmtId); + matchBoolCondition(Finder, false, ConditionElseStmtId); + + matchTernaryResult(Finder, true, TernaryId); + matchTernaryResult(Finder, false, TernaryNegatedId); + + matchIfReturnsBool(Finder, true, IfReturnsBoolId); + matchIfReturnsBool(Finder, false, IfReturnsNotBoolId); + + matchIfAssignsBool(Finder, true, IfAssignBoolId); + matchIfAssignsBool(Finder, false, IfAssignNotBoolId); +} + +void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) { + if (const CXXBoolLiteralExpr *LeftRemoved = + getBoolLiteral(Result, RightExpressionId)) { + replaceWithExpression(Result, LeftRemoved, false); + } else if (const CXXBoolLiteralExpr *RightRemoved = + getBoolLiteral(Result, LeftExpressionId)) { + replaceWithExpression(Result, RightRemoved, true); + } else if (const CXXBoolLiteralExpr *NegatedLeftRemoved = + getBoolLiteral(Result, NegatedRightExpressionId)) { + replaceWithExpression(Result, NegatedLeftRemoved, false, true); + } else if (const CXXBoolLiteralExpr *NegatedRightRemoved = + getBoolLiteral(Result, NegatedLeftExpressionId)) { + replaceWithExpression(Result, NegatedRightRemoved, true, true); + } else if (const CXXBoolLiteralExpr *TrueConditionRemoved = + getBoolLiteral(Result, ConditionThenStmtId)) { + replaceWithThenStatement(Result, TrueConditionRemoved); + } else if (const CXXBoolLiteralExpr *FalseConditionRemoved = + getBoolLiteral(Result, ConditionElseStmtId)) { + replaceWithElseStatement(Result, FalseConditionRemoved); + } else if (const auto *Ternary = + Result.Nodes.getNodeAs(TernaryId)) { + replaceWithCondition(Result, Ternary); + } else if (const auto *TernaryNegated = + Result.Nodes.getNodeAs( + TernaryNegatedId)) { + replaceWithCondition(Result, TernaryNegated, true); + } else if (const auto *If = Result.Nodes.getNodeAs(IfReturnsBoolId)) { + replaceWithReturnCondition(Result, If); + } else if (const auto *IfNot = + Result.Nodes.getNodeAs(IfReturnsNotBoolId)) { + replaceWithReturnCondition(Result, IfNot, true); + } else if (const auto *IfAssign = + Result.Nodes.getNodeAs(IfAssignBoolId)) { + replaceWithAssignment(Result, IfAssign); + } else if (const auto *IfAssignNot = + Result.Nodes.getNodeAs(IfAssignNotBoolId)) { + replaceWithAssignment(Result, IfAssignNot, true); + } +} + +void SimplifyBooleanExprCheck::replaceWithExpression( + const ast_matchers::MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, bool Negated) { + const auto *LHS = Result.Nodes.getNodeAs(LHSId); + const auto *RHS = Result.Nodes.getNodeAs(RHSId); + std::string Replacement = + replacementExpression(Result, Negated, UseLHS ? LHS : RHS); + SourceLocation Start = LHS->getLocStart(); + SourceLocation End = RHS->getLocEnd(); + diag(BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic) + << FixItHint::CreateReplacement(SourceRange(Start, End), Replacement); +} + +void SimplifyBooleanExprCheck::replaceWithThenStatement( + const MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *TrueConditionRemoved) { + const auto *IfStatement = Result.Nodes.getNodeAs(IfStmtId); + diag(TrueConditionRemoved->getLocStart(), SimplifyConditionDiagnostic) + << FixItHint::CreateReplacement(IfStatement->getSourceRange(), + getText(Result, *IfStatement->getThen())); +} + +void SimplifyBooleanExprCheck::replaceWithElseStatement( + const MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *FalseConditionRemoved) { + const auto *IfStatement = Result.Nodes.getNodeAs(IfStmtId); + const Stmt *ElseStatement = IfStatement->getElse(); + diag(FalseConditionRemoved->getLocStart(), SimplifyConditionDiagnostic) + << FixItHint::CreateReplacement( + IfStatement->getSourceRange(), + ElseStatement ? getText(Result, *ElseStatement) : ""); +} + +void SimplifyBooleanExprCheck::replaceWithCondition( + const MatchFinder::MatchResult &Result, const ConditionalOperator *Ternary, + bool Negated) { + std::string Replacement = + replacementExpression(Result, Negated, Ternary->getCond()); + diag(Ternary->getTrueExpr()->getLocStart(), + "redundant boolean literal in ternary expression result") + << FixItHint::CreateReplacement(Ternary->getSourceRange(), Replacement); +} + +void SimplifyBooleanExprCheck::replaceWithReturnCondition( + const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) { + StringRef Terminator = isa(If->getElse()) ? ";" : ""; + std::string Condition = replacementExpression(Result, Negated, If->getCond()); + std::string Replacement = ("return " + Condition + Terminator).str(); + SourceLocation Start = + Result.Nodes.getNodeAs(ThenLiteralId)->getLocStart(); + diag(Start, "redundant boolean literal in conditional return statement") + << FixItHint::CreateReplacement(If->getSourceRange(), Replacement); +} + +void SimplifyBooleanExprCheck::replaceWithAssignment( + const MatchFinder::MatchResult &Result, const IfStmt *IfAssign, + bool Negated) { + SourceRange Range = IfAssign->getSourceRange(); + StringRef VariableName = + getText(Result, *Result.Nodes.getNodeAs(IfAssignVariableId)); + StringRef Terminator = isa(IfAssign->getElse()) ? ";" : ""; + std::string Condition = + replacementExpression(Result, Negated, IfAssign->getCond()); + std::string Replacement = + (VariableName + " = " + Condition + Terminator).str(); + SourceLocation Location = + Result.Nodes.getNodeAs(IfAssignLocId)->getLocStart(); + this->diag(Location, "redundant boolean literal in conditional assignment") + << FixItHint::CreateReplacement(Range, Replacement); +} + +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h new file mode 100644 index 000000000000..5bb67a47bfc6 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -0,0 +1,101 @@ +//===--- SimplifyBooleanExpr.h clang-tidy -----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// \brief Looks for boolean expressions involving boolean constants and +// simplifies them to use the appropriate boolean expression directly. +/// +/// Examples: +/// `if (b == true)` becomes `if (b)` +/// `if (b == false)` becomes `if (!b)` +/// `if (b && true)` becomes `if (b)` +/// `if (b && false)` becomes `if (false)` +/// `if (b || true)` becomes `if (true)` +/// `if (b || false)` becomes `if (b)` +/// `e ? true : false` becomes `e` +/// `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) b = true; else b = false;` becomes `b = e;` +/// `if (e) b = false; else b = true;` becomes `b = !(e);` +/// +class SimplifyBooleanExprCheck : public ClangTidyCheck { +public: + SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void matchBoolBinOpExpr(ast_matchers::MatchFinder *Finder, bool Value, + StringRef OperatorName, StringRef BooleanId); + + void matchExprBinOpBool(ast_matchers::MatchFinder *Finder, bool Value, + StringRef OperatorName, StringRef BooleanId); + + void matchBoolCompOpExpr(ast_matchers::MatchFinder *Finder, bool Value, + StringRef OperatorName, StringRef BooleanId); + + void matchExprCompOpBool(ast_matchers::MatchFinder *Finder, bool Value, + StringRef OperatorName, StringRef BooleanId); + + void matchBoolCondition(ast_matchers::MatchFinder *Finder, bool Value, + StringRef BooleanId); + + void matchTernaryResult(ast_matchers::MatchFinder *Finder, bool Value, + StringRef TernaryId); + + void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value, + StringRef Id); + + void matchIfAssignsBool(ast_matchers::MatchFinder *Finder, bool Value, + StringRef Id); + + void + replaceWithExpression(const ast_matchers::MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, + bool Negated = false); + + void + replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *BoolLiteral); + + void + replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult &Result, + const CXXBoolLiteralExpr *FalseConditionRemoved); + + void + replaceWithCondition(const ast_matchers::MatchFinder::MatchResult &Result, + const ConditionalOperator *Ternary, + bool Negated = false); + + void replaceWithReturnCondition( + const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If, + bool Negated = false); + + void + replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result, + const IfStmt *If, bool Negated = false); +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H diff --git a/clang-tools-extra/test/clang-tidy/readability-simplify-bool-expr.cpp b/clang-tools-extra/test/clang-tidy/readability-simplify-bool-expr.cpp new file mode 100644 index 000000000000..0658a17b2787 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/readability-simplify-bool-expr.cpp @@ -0,0 +1,543 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t +// REQUIRES: shell + +bool a1 = false; + +//=-=-=-=-=-=-= operator == +bool aa = false == a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr] +// CHECK-FIXES: {{^bool aa = !a1;$}} +bool ab = true == a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool ab = a1;$}} +bool a2 = a1 == false; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a2 = !a1;$}} +bool a3 = a1 == true; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a3 = a1;$}} + +//=-=-=-=-=-=-= operator != +bool n1 = a1 != false; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool n1 = a1;$}} +bool n2 = a1 != true; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool n2 = !a1;$}} +bool n3 = false != a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool n3 = a1;$}} +bool n4 = true != a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool n4 = !a1;$}} + +//=-=-=-=-=-=-= operator || +bool a4 = a1 || false; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a4 = a1;$}} +bool a5 = a1 || true; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a5 = true;$}} +bool a6 = false || a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a6 = a1;$}} +bool a7 = true || a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a7 = true;$}} + +//=-=-=-=-=-=-= operator && +bool a8 = a1 && false; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a8 = false;$}} +bool a9 = a1 && true; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool a9 = a1;$}} +bool ac = false && a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool ac = false;$}} +bool ad = true && a1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: {{.*}} to boolean operator +// CHECK-FIXES: {{^bool ad = a1;$}} + +void if_with_bool_literal_condition() { + int i = 0; + if (false) { + i = 1; + } else { + i = 2; + } + i = 3; + // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition + // CHECK-FIXES: {{^ int i = 0;$}} + // CHECK-FIXES-NEXT: {{^ {$}} + // CHECK-FIXES-NEXT: {{^ i = 2;$}} + // CHECK-FIXES-NEXT: {{^ }$}} + // CHECK-FIXES-NEXT: {{^ i = 3;$}} + + i = 4; + if (true) { + i = 5; + } else { + i = 6; + } + i = 7; + // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition + // CHECK-FIXES: {{^ i = 4;$}} + // CHECK-FIXES-NEXT: {{^ {$}} + // CHECK-FIXES-NEXT: {{^ i = 5;$}} + // CHECK-FIXES-NEXT: {{^ }$}} + // CHECK-FIXES-NEXT: {{^ i = 7;$}} + + i = 8; + if (false) { + i = 9; + } + i = 11; + // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition + // CHECK-FIXES: {{^ i = 8;$}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ i = 11;$}} +} + +void operator_equals() { + int i = 0; + bool b1 = (i > 2); + if (b1 == true) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(b1\) {$}} + i = 5; + } else { + i = 6; + } + bool b2 = (i > 4); + if (b2 == false) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(!b2\) {$}} + i = 7; + } else { + i = 9; + } + bool b3 = (i > 6); + if (true == b3) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(b3\) {$}} + i = 10; + } else { + i = 11; + } + bool b4 = (i > 8); + if (false == b4) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(!b4\) {$}} + i = 12; + } else { + i = 13; + } +} + +void operator_or() { + int i = 0; + bool b5 = (i > 10); + if (b5 || false) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(b5\) {$}} + i = 14; + } else { + i = 15; + } + bool b6 = (i > 10); + if (b6 || true) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(true\) {$}} + i = 16; + } else { + i = 17; + } + bool b7 = (i > 10); + if (false || b7) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(b7\) {$}} + i = 18; + } else { + i = 19; + } + bool b8 = (i > 10); + if (true || b8) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(true\) {$}} + i = 20; + } else { + i = 21; + } +} + +void operator_and() { + int i = 0; + bool b9 = (i > 20); + if (b9 && false) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(false\) {$}} + i = 22; + } else { + i = 23; + } + bool ba = (i > 20); + if (ba && true) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(ba\) {$}} + i = 24; + } else { + i = 25; + } + bool bb = (i > 20); + if (false && bb) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(false\) {$}} + i = 26; + } else { + i = 27; + } + bool bc = (i > 20); + if (true && bc) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(bc\) {$}} + i = 28; + } else { + i = 29; + } +} + +void ternary_operator() { + int i = 0; + bool bd = (i > 20) ? true : false; + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} in ternary expression result + // CHECK-FIXES: {{^ bool bd = i > 20;$}} + + bool be = (i > 20) ? false : true; + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} in ternary expression result + // CHECK-FIXES: {{^ bool be = i <= 20;$}} + + bool bf = ((i > 20)) ? false : true; + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: {{.*}} in ternary expression result + // CHECK-FIXES: {{^ bool bf = i <= 20;$}} +} + +void operator_not_equal() { + int i = 0; + bool bf = (i > 20); + if (false != bf) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(bf\) {$}} + i = 30; + } else { + i = 31; + } + bool bg = (i > 20); + if (true != bg) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(!bg\) {$}} + i = 32; + } else { + i = 33; + } + bool bh = (i > 20); + if (bh != false) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(bh\) {$}} + i = 34; + } else { + i = 35; + } + bool bi = (i > 20); + if (bi != true) { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(!bi\) {$}} + i = 36; + } else { + i = 37; + } +} + +void nested_booleans() { + if (false || (true || false)) { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(false \|\| \(true\)\) {$}} + } + if (true && (true || false)) { + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(true && \(true\)\) {$}} + } + if (false || (true && false)) { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(false \|\| \(false\)\) {$}} + } + if (true && (true && false)) { + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*}} to boolean operator + // CHECK-FIXES: {{^ if \(true && \(false\)\) {$}} + } +} + +static constexpr bool truthy() { + return true; +} + +#define HAS_XYZ_FEATURE true + +void macros_and_constexprs(int i = 0) { + bool b = (i == 1); + if (b && truthy()) { + // leave this alone; if you want it simplified, then you should + // inline the constexpr function first. + i = 1; + } + i = 2; + if (b && HAS_XYZ_FEATURE) { + // leave this alone; if you want it simplified, then you should + // inline the macro first. + i = 3; + } + i = 4; +} + +bool conditional_return_statements(int i) { + if (i == 0) return true; else return false; +} +// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: {{.*}} in conditional return statement +// CHECK-FIXES: {{^}} return i == 0;{{$}} +// CHECK-FIXES-NEXT: {{^}$}} + +bool conditional_return_statements_then_expr(int i, int j) { + if (i == j) return (i == 0); else return false; +} + +bool conditional_return_statements_else_expr(int i, int j) { + if (i == j) return true; else return (i == 0); +} + +bool negated_conditional_return_statements(int i) { + if (i == 0) return false; else return true; +} +// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: {{.*}} in conditional return statement +// CHECK-FIXES: {{^}} return i != 0;{{$}} +// CHECK-FIXES-NEXT: {{^}$}} + +bool conditional_compound_return_statements(int i) { + if (i == 1) { + return true; + } else { + return false; + } +} +// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return statement +// CHECK-FIXES: {{^}}bool conditional_compound_return_statements(int i) {{{$}} +// CHECK-FIXES-NEXT: {{^}} return i == 1;{{$}} +// CHECK-FIXES-NEXT: {{^}$}} + +bool negated_conditional_compound_return_statements(int i) { + if (i == 1) { + return false; + } else { + return true; + } +} +// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return statement +// CHECK-FIXES: {{^}}bool negated_conditional_compound_return_statements(int i) {{{$}} +// CHECK-FIXES-NEXT: {{^}} return i != 1;{{$}} +// CHECK-FIXES-NEXT: {{^}$}} + +bool conditional_return_statements_side_effects_then(int i) { + if (i == 2) { + macros_and_constexprs(); + return true; + } else + return false; +} + +bool negated_conditional_return_statements_side_effects_then(int i) { + if (i == 2) { + macros_and_constexprs(); + return false; + } else + return true; +} + +bool conditional_return_statements_side_effects_else(int i) { + if (i == 2) + return true; + else { + macros_and_constexprs(); + return false; + } +} + +bool negated_conditional_return_statements_side_effects_else(int i) { + if (i == 2) + return false; + else { + macros_and_constexprs(); + return true; + } +} + +void lambda_conditional_return_statements() { + auto lambda = [](int n) -> bool { if (n > 0) return true; else return false; }; + // CHECK-MESSAGES: :[[@LINE-1]]:55: warning: {{.*}} in conditional return statement + // CHECK-FIXES: {{^}} auto lambda = [](int n) -> bool { return n > 0; };{{$}} + + auto lambda2 = [](int n) -> bool { + if (n > 0) { + return true; + } else { + return false; + } + }; + // CHECK-MESSAGES: :[[@LINE-5]]:16: warning: {{.*}} in conditional return statement + // CHECK-FIXES: {{^}} auto lambda2 = [](int n) -> bool {{{$}} + // CHECK-FIXES-NEXT: {{^}} return n > 0;{{$}} + // CHECK-FIXES-NEXT: {{^}} };{{$}} + + auto lambda3 = [](int n) -> bool { if (n > 0) {macros_and_constexprs(); return true; } else return false; }; + + auto lambda4 = [](int n) -> bool { + if (n > 0) + return true; + else { + macros_and_constexprs(); + return false; + } + }; + + auto lambda5 = [](int n) -> bool { if (n > 0) return false; else return true; }; + // CHECK-MESSAGES: :[[@LINE-1]]:56: warning: {{.*}} in conditional return statement + // CHECK-FIXES: {{^}} auto lambda5 = [](int n) -> bool { return n <= 0; };{{$}} + + auto lambda6 = [](int n) -> bool { + if (n > 0) { + return false; + } else { + return true; + } + }; + // CHECK-MESSAGES: :[[@LINE-5]]:16: warning: {{.*}} in conditional return statement + // CHECK-FIXES: {{^}} auto lambda6 = [](int n) -> bool {{{$}} + // CHECK-FIXES-NEXT: {{^}} return n <= 0;{{$}} + // CHECK-FIXES-NEXT: {{^}} };{{$}} +} + +void simple_conditional_assignment_statements(int i) { + bool b; + if (i > 10) + b = true; + else + b = false; + bool bb = false; + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment + // CHECK-FIXES: bool b; + // CHECK-FIXES: {{^ }}b = i > 10;{{$}} + // CHECK-FIXES: bool bb = false; + + bool c; + if (i > 20) + c = false; + else + c = true; + bool c2 = false; + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment + // CHECK-FIXES: bool c; + // CHECK-FIXES: {{^ }}c = i <= 20;{{$}} + // CHECK-FIXES: bool c2 = false; + + // unchanged; different variables + bool b2; + if (i > 12) + b = true; + else + b2 = false; + + // unchanged; no else statement + bool b3; + if (i > 15) + b3 = true; + + // unchanged; not boolean assignment + int j; + if (i > 17) + j = 10; + else + j = 20; + + // unchanged; different variables assigned + int k = 0; + bool b4 = false; + if (i > 10) + b4 = true; + else + k = 10; +} + +void complex_conditional_assignment_statements(int i) { + bool d; + if (i > 30) { + d = true; + } else { + d = false; + } + d = false; + // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in conditional assignment + // CHECK-FIXES: bool d; + // CHECK-FIXES: {{^ }}d = i > 30;{{$}} + // CHECK-FIXES: d = false; + + bool e; + if (i > 40) { + e = false; + } else { + e = true; + } + e = false; + // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in conditional assignment + // CHECK-FIXES: bool e; + // CHECK-FIXES: {{^ }}e = i <= 40;{{$}} + // CHECK-FIXES: e = false; + + // unchanged; no else statement + bool b3; + if (i > 15) { + b3 = true; + } + + // unchanged; not a boolean assignment + int j; + if (i > 17) { + j = 10; + } else { + j = 20; + } + + // unchanged; multiple statements + bool f; + if (j > 10) { + j = 10; + f = true; + } else { + j = 20; + f = false; + } + + // unchanged; multiple statements + bool g; + if (j > 10) + f = true; + else { + j = 20; + f = false; + } + + // unchanged; multiple statements + bool h; + if (j > 10) { + j = 10; + f = true; + } else + f = false; +}