[clang-tidy][NFC] Reimplement SimplifyBooleanExpr with RecursiveASTVisitors

[njames93]

Summary

Reimplement the matching logic using Visitors instead of matchers.

Benchmarks from running the check over SemaCodeComplete.cpp
Before 0.20s, After 0.04s

[njames93]

Remove redundant traversal of CompoundStmt in a replace method.

[njames93]

Transform final matcher, Benchmark -> 0.82s.

[njames93]

Remove SimplifyBoolExprMatchers header

[njames93]

Remove unnecessary tests in ReadabilityTidyModule now that the SimplifyBooleanExprMatchers header has been removed.

[njames93]

Clean up some code.

[njames93]

Tweak logic for detecting nested if.

[njames93]

Nit.

[njames93]

Since latest update speed up gone from 0.87s to 0.04s for SemaCodeComplete.

[aaron.ballman]

LGTM, thank you for this!

[njames93]

So those times were a little unfair as now we don't use ParentMapContext, however the cost of building that is included in the running time for the check.
In a use case where you have other enabled checks with do make use of that, the cost for building would be moved to other checks.
In order to level the playing field slightly, I've build the ParentMapContext ahead of time in both implementations for a fairer comparison in typical use cases.
This yields 0.20s for the original impl and 0.04s for this implementation.
Not quite as large a gap but still a 5x improvement.

[LegalizeAdulthood]

Just for my own edification, how did you know/suspect that a pure visitor would be faster than matchers?

[njames93]

In D125026#3519831, @LegalizeAdulthood wrote:

Just for my own edification, how did you know/suspect that a pure visitor would be faster than matchers?

Mainly cause the fact we are creating 2 matcher expressions that differ by a bool value for each pattern, a visitor can easily handle both cases in one go.
There's also overhead with ASTMatchers that for simple cases sometimes may not be worth it.
It's not all good news though, there's a cost associated with running another ASTTraversal, however as we already have a visitor in this check we don't pay for that cost again.

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h

public:
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
     return TK_IgnoreUnlessSpelledInSource;
   }
 
 private:
   class Visitor;
 
-  void reportBinOp(const ast_matchers::MatchFinder::MatchResult &Result,
-                   const BinaryOperator *Op);
-
-  void matchBoolCondition(ast_matchers::MatchFinder *Finder, bool Value,
-                          StringRef BooleanId);
-
-  void matchTernaryResult(ast_matchers::MatchFinder *Finder, bool Value,
-                          StringRef Id);
-
-  void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
-                          StringRef Id);
+  void reportBinOp(const ASTContext &Context, const BinaryOperator *Op);
 
   void matchIfAssignsBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef Id);
 
-  void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
-                                  StringRef Id);
-
-  void matchCaseIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
-                              StringRef Id);
-
-  void matchDefaultIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
-                                 StringRef Id);
-
-  void matchLabelIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
-                               StringRef Id);
-
-  void
-  replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
+  void replaceWithThenStatement(const ASTContext &Context,
+                                const IfStmt *IfStatement,
                                 const Expr *BoolLiteral);
 
-  void
-  replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult &Result,
+  void replaceWithElseStatement(const ASTContext &Context,
+                                const IfStmt *IfStatement,
                                 const Expr *BoolLiteral);
 
-  void
-  replaceWithCondition(const ast_matchers::MatchFinder::MatchResult &Result,
+  void replaceWithCondition(const ASTContext &Context,
                             const ConditionalOperator *Ternary, bool Negated);
 
-  void replaceWithReturnCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,
-      bool Negated);
+  void replaceWithReturnCondition(const ASTContext &Context, const IfStmt *If,
+                                  const Expr *BoolLiteral, bool Negated);
 
   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
                         const IfStmt *If, bool Negated);
 
-  void replaceCompoundReturnWithCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result,
-      const CompoundStmt *Compound, bool Negated);
+  void replaceCompoundReturnWithCondition(const ASTContext &Context,
+                                          const CompoundStmt *Compound,
+                                          const ReturnStmt *Ret, bool Negated);
 
-  void replaceCompoundReturnWithCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result, bool Negated,
+  void replaceCompoundReturnWithCondition(const ASTContext &Context,
+                                          const ReturnStmt *Ret, bool Negated,
                                           const IfStmt *If);
 
-  void replaceCaseCompoundReturnWithCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
-
-  void replaceDefaultCompoundReturnWithCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
-
-  void replaceLabelCompoundReturnWithCondition(
-      const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
-
-  void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result,
-                 SourceLocation Loc, StringRef Description,
-                 SourceRange ReplacementRange, StringRef Replacement);
+  void issueDiag(const ASTContext &Result, SourceLocation Loc,
+                 StringRef Description, SourceRange ReplacementRange,
+                 StringRef Replacement);
 
   const bool ChainedConditionalReturn;
   const bool ChainedConditionalAssignment;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

 //===-- SimplifyBooleanExprCheck.cpp - clang-tidy -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SimplifyBooleanExprCheck.h"
 #include "SimplifyBooleanExprMatchers.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/PointerIntPair.h"
 
 #include <string>
 #include <utility>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 namespace {
 
-StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+StringRef getText(const ASTContext &Context, SourceRange Range) {
   return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
-                              *Result.SourceManager,
-                              Result.Context->getLangOpts());
+                              Context.getSourceManager(),
+                              Context.getLangOpts());
 }
 
-template <typename T>
-StringRef getText(const MatchFinder::MatchResult &Result, T &Node) {
-  return getText(Result, Node.getSourceRange());
+template <typename T> StringRef getText(const ASTContext &Context, T &Node) {
+  return getText(Context, Node.getSourceRange());
 }
 
 } // namespace
 
-static constexpr char ConditionThenStmtId[] = "if-bool-yields-then";
-static constexpr char ConditionElseStmtId[] = "if-bool-yields-else";
-static constexpr char TernaryId[] = "ternary-bool-yields-condition";
-static constexpr char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
-static constexpr char IfReturnsBoolId[] = "if-return";
-static constexpr char IfReturnsNotBoolId[] = "if-not-return";
-static constexpr char ThenLiteralId[] = "then-literal";
 static constexpr char IfAssignVariableId[] = "if-assign-lvalue";
 static constexpr char IfAssignLocId[] = "if-assign-loc";
 static constexpr char IfAssignBoolId[] = "if-assign";
 static constexpr char IfAssignNotBoolId[] = "if-assign-not";
 static constexpr char IfAssignVarId[] = "if-assign-var";
-static constexpr char CompoundReturnId[] = "compound-return";
-static constexpr char CompoundIfId[] = "compound-if";
-static constexpr char CompoundBoolId[] = "compound-bool";
-static constexpr char CompoundNotBoolId[] = "compound-bool-not";
-static constexpr char CaseId[] = "case";
-static constexpr char CaseCompoundBoolId[] = "case-compound-bool";
-static constexpr char CaseCompoundNotBoolId[] = "case-compound-bool-not";
-static constexpr char DefaultId[] = "default";
-static constexpr char DefaultCompoundBoolId[] = "default-compound-bool";
-static constexpr char DefaultCompoundNotBoolId[] = "default-compound-bool-not";
-static constexpr char LabelId[] = "label";
-static constexpr char LabelCompoundBoolId[] = "label-compound-bool";
-static constexpr char LabelCompoundNotBoolId[] = "label-compound-bool-not";
-static constexpr char IfStmtId[] = "if";
 
 static constexpr char SimplifyOperatorDiagnostic[] =
     "redundant boolean literal supplied to boolean operator";
 static constexpr char SimplifyConditionDiagnostic[] =
     "redundant boolean literal in if statement condition";
 static constexpr char SimplifyConditionalReturnDiagnostic[] =
     "redundant boolean literal in conditional return statement";
 
-static const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
-                                  StringRef Id) {
-  if (const Expr *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id))
-    return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
-  if (const auto *Negated = Result.Nodes.getNodeAs<UnaryOperator>(Id)) {
-    if (Negated->getOpcode() == UO_LNot &&
-        isa<CXXBoolLiteralExpr>(Negated->getSubExpr()))
-      return Negated->getBeginLoc().isMacroID() ? nullptr : Negated;
-  }
-  return nullptr;
-}
-
 static internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
   return expr(
       anyOf(cxxBoolLiteral(equals(Value)),
             unaryOperator(hasUnaryOperand(cxxBoolLiteral(equals(!Value))),
                           hasOperatorName("!"))));
 }
 
-static internal::Matcher<Stmt> returnsBool(bool Value,
-                                           StringRef Id = "ignored") {
-  auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
-                               .bind("returns-bool");
-  return anyOf(SimpleReturnsBool,
-               compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
-}
-
 static bool needsParensAfterUnaryNegation(const Expr *E) {
   E = E->IgnoreImpCasts();
   if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
     return true;
 
   if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
     return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
            Op->getOperator() != OO_Subscript;
if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
       }
     }
   }
 
   E = E->IgnoreImpCasts();
   return !E->getType()->isBooleanType();
 }
 
-static std::string
-compareExpressionToConstant(const MatchFinder::MatchResult &Result,
-                            const Expr *E, bool Negated, const char *Constant) {
+static std::string compareExpressionToConstant(const ASTContext &Context,
+                                               const Expr *E, bool Negated,
+                                               const char *Constant) {
   E = E->IgnoreImpCasts();
   const std::string ExprText =
-      (isa<BinaryOperator>(E) ? ("(" + getText(Result, *E) + ")")
-                              : getText(Result, *E))
+      (isa<BinaryOperator>(E) ? ("(" + getText(Context, *E) + ")")
+                              : getText(Context, *E))
           .str();
   return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant;
 }
 
-static std::string
-compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
+static std::string compareExpressionToNullPtr(const ASTContext &Context,
                                               const Expr *E, bool Negated) {
-  const char *NullPtr =
-      Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
-  return compareExpressionToConstant(Result, E, Negated, NullPtr);
+  const char *NullPtr = Context.getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
+  return compareExpressionToConstant(Context, E, Negated, NullPtr);
 }
 
-static std::string
-compareExpressionToZero(const MatchFinder::MatchResult &Result, const Expr *E,
-                        bool Negated) {
-  return compareExpressionToConstant(Result, E, Negated, "0");
+static std::string compareExpressionToZero(const ASTContext &Context,
+                                           const Expr *E, bool Negated) {
+  return compareExpressionToConstant(Context, E, Negated, "0");
 }
 
-static std::string replacementExpression(const MatchFinder::MatchResult &Result,
+static std::string replacementExpression(const ASTContext &Context,
                                          bool Negated, const Expr *E) {
   E = E->IgnoreParenBaseCasts();
   if (const auto *EC = dyn_cast<ExprWithCleanups>(E))
     E = EC->getSubExpr();
 
   const bool NeedsStaticCast = needsStaticCast(E);
   if (Negated) {
     if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
       if (UnOp->getOpcode() == UO_LNot) {
         if (needsNullPtrComparison(UnOp->getSubExpr()))
-          return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), true);
+          return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), true);
 
         if (needsZeroComparison(UnOp->getSubExpr()))
-          return compareExpressionToZero(Result, UnOp->getSubExpr(), true);
+          return compareExpressionToZero(Context, UnOp->getSubExpr(), true);
 
-        return replacementExpression(Result, false, UnOp->getSubExpr());
+        return replacementExpression(Context, false, UnOp->getSubExpr());
       }
     }
 
     if (needsNullPtrComparison(E))
-      return compareExpressionToNullPtr(Result, E, false);
+      return compareExpressionToNullPtr(Context, E, false);
 
     if (needsZeroComparison(E))
-      return compareExpressionToZero(Result, E, false);
+      return compareExpressionToZero(Context, E, false);
 
     StringRef NegatedOperator;
     const Expr *LHS = nullptr;
     const Expr *RHS = nullptr;
     if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
       NegatedOperator = negatedOperator(BinOp);
       LHS = BinOp->getLHS();
       RHS = BinOp->getRHS();
     } else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
       if (OpExpr->getNumArgs() == 2) {
         NegatedOperator = negatedOperator(OpExpr);
         LHS = OpExpr->getArg(0);
         RHS = OpExpr->getArg(1);
       }
     }
     if (!NegatedOperator.empty() && LHS && RHS)
-      return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " +
-                      getText(Result, *RHS))
+      return (asBool((getText(Context, *LHS) + " " + NegatedOperator + " " +
+                      getText(Context, *RHS))
                          .str(),
                      NeedsStaticCast));
 
-    StringRef Text = getText(Result, *E);
+    StringRef Text = getText(Context, *E);
     if (!NeedsStaticCast && needsParensAfterUnaryNegation(E))
       return ("!(" + Text + ")").str();
 
     if (needsNullPtrComparison(E))
-      return compareExpressionToNullPtr(Result, E, false);
+      return compareExpressionToNullPtr(Context, E, false);
 
     if (needsZeroComparison(E))
-      return compareExpressionToZero(Result, E, false);
+      return compareExpressionToZero(Context, E, false);
 
     return ("!" + asBool(Text, NeedsStaticCast));
   }
 
   if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
     if (UnOp->getOpcode() == UO_LNot) {
       if (needsNullPtrComparison(UnOp->getSubExpr()))
-        return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), false);
+        return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), false);
 
       if (needsZeroComparison(UnOp->getSubExpr()))
-        return compareExpressionToZero(Result, UnOp->getSubExpr(), false);
+        return compareExpressionToZero(Context, UnOp->getSubExpr(), false);
     }
   }
 
   if (needsNullPtrComparison(E))
-    return compareExpressionToNullPtr(Result, E, true);
+    return compareExpressionToNullPtr(Context, E, true);
 
   if (needsZeroComparison(E))
-    return compareExpressionToZero(Result, E, true);
+    return compareExpressionToZero(Context, E, true);
 
-  return asBool(getText(Result, *E), NeedsStaticCast);
+  return asBool(getText(Context, *E), NeedsStaticCast);
 }
 
 static const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
   if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
     if (Bool->getValue() == !Negated)
       return Bool;
   }
   if (const auto *Unary = dyn_cast<UnaryOperator>(Ret->getRetValue())) {
if (Compound->size() == 1) {
       if (const auto *CompoundRet = dyn_cast<ReturnStmt>(Compound->body_back()))
         return stmtReturnsBool(CompoundRet, Negated);
     }
   }
 
   return nullptr;
 }
 
-static bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
+static bool containsDiscardedTokens(const ASTContext &Context,
                                     CharSourceRange CharRange) {
   std::string ReplacementText =
-      Lexer::getSourceText(CharRange, *Result.SourceManager,
-                           Result.Context->getLangOpts())
+      Lexer::getSourceText(CharRange, Context.getSourceManager(),
+                           Context.getLangOpts())
           .str();
-  Lexer Lex(CharRange.getBegin(), Result.Context->getLangOpts(),
-            ReplacementText.data(), ReplacementText.data(),
+  Lexer Lex(CharRange.getBegin(), 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;
 }
 
 class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
 public:
-  Visitor(SimplifyBooleanExprCheck *Check,
-          const MatchFinder::MatchResult &Result)
-      : Check(Check), Result(Result) {}
+  Visitor(SimplifyBooleanExprCheck *Check, ASTContext &Context)
+      : Check(Check), Context(Context) {}
+
+  bool traverse() { return TraverseAST(Context); }
 
   bool VisitBinaryOperator(const BinaryOperator *Op) const {
-    Check->reportBinOp(Result, Op);
+    Check->reportBinOp(Context, Op);
+    return true;
+  }
+
+  static Optional<bool> getAsBoolLiteral(const Expr *E, bool FilterMacro) {
+    if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(E)) {
+      if (FilterMacro && Bool->getBeginLoc().isMacroID())
+        return llvm::None;
+      return Bool->getValue();
+    }
+    if (const auto *UOp = dyn_cast<UnaryOperator>(E)) {
+      if (FilterMacro && UOp->getBeginLoc().isMacroID())
+        return None;
+      if (UOp->getOpcode() == UO_LNot)
+        if (Optional<bool> Res = getAsBoolLiteral(
+                UOp->getSubExpr()->IgnoreImplicit(), FilterMacro))
+          return !*Res;
+    }
+    return llvm::None;
+  }
+
+  static llvm::PointerIntPair<const Expr *, 1, bool>
+  parseReturnLiteralBool(const Stmt *S) {
+    const auto *RS = dyn_cast<ReturnStmt>(S);
+    if (!RS || !RS->getRetValue())
+      return {};
+    if (auto Ret =
+            getAsBoolLiteral(RS->getRetValue()->IgnoreImplicit(), false)) {
+      return {RS->getRetValue(), *Ret};
+    }
+    return {};
+  }
+
+  template <typename Functor>
+  static auto checkSingleStatement(Stmt *S, Functor F) -> decltype(F(S)) {
+    if (auto *CS = dyn_cast<CompoundStmt>(S)) {
+      if (CS->size() == 1)
+        return F(CS->body_front());
+      return {};
+    }
+    return F(S);
+  }
+
+  bool doesIfHaveIfParent(const IfStmt *If) {
+    auto Parents = Context.getParents(*If);
+    if (Parents.empty())
+      return false;
+    return Parents[0].get<IfStmt>() != nullptr;
+  }
+
+  bool VisitIfStmt(IfStmt *If) {
+    Expr *Cond = If->getCond()->IgnoreImplicit();
+    if (auto Bool = getAsBoolLiteral(Cond, true)) {
+      if (*Bool)
+        Check->replaceWithThenStatement(Context, If, Cond);
+      else
+        Check->replaceWithElseStatement(Context, If, Cond);
+    }
+
+    if (If->getElse()) {
+      auto ThenReturnBool =
+          checkSingleStatement(If->getThen(), parseReturnLiteralBool);
+      if (ThenReturnBool.getPointer()) {
+        auto ElseReturnBool =
+            checkSingleStatement(If->getElse(), parseReturnLiteralBool);
+        if (ElseReturnBool.getPointer() &&
+            ThenReturnBool.getInt() != ElseReturnBool.getInt()) {
+          if (Check->ChainedConditionalReturn || !doesIfHaveIfParent(If)) {
+            Check->replaceWithReturnCondition(Context, If,
+                                              ThenReturnBool.getPointer(),
+                                              !ThenReturnBool.getInt());
+          }
+        }
+      }
+    }
+    return true;
+  }
+
+  bool VisitConditionalOperator(ConditionalOperator *Cond) {
+    if (auto Then =
+            getAsBoolLiteral(Cond->getTrueExpr()->IgnoreImplicit(), false)) {
+      if (auto Else =
+              getAsBoolLiteral(Cond->getFalseExpr()->IgnoreImplicit(), false)) {
+        if (*Then != *Else)
+          Check->replaceWithCondition(Context, Cond, *Else);
+      }
+    }
+    return true;
+  }
+
+  bool VisitCompoundStmt(CompoundStmt *CS) {
+    if (CS->size() < 2)
+      return true;
+    for (auto Second = CS->body_rbegin(), First = std::next(Second),
+              End = CS->body_rend();
+         First != End; ++Second, ++First) {
+      auto RetStmt = parseReturnLiteralBool(*Second);
+      if (!RetStmt.getPointer())
+        continue;
+
+      if (auto *If = dyn_cast<IfStmt>(*First)) {
+        auto ThenReturnBool =
+            checkSingleStatement(If->getThen(), parseReturnLiteralBool);
+        if (ThenReturnBool.getPointer() &&
+            ThenReturnBool.getInt() != RetStmt.getInt()) {
+          if (Check->ChainedConditionalReturn ||
+              (If->getElse() == nullptr && !doesIfHaveIfParent(If))) {
+            Check->replaceCompoundReturnWithCondition(
+                Context, CS, cast<ReturnStmt>(*Second), RetStmt.getInt());
+          }
+        }
+      } else if (isa<LabelStmt, CaseStmt, DefaultStmt>(*First)) {
+        Stmt *SubStmt =
+            isa<LabelStmt>(*First)  ? cast<LabelStmt>(*First)->getSubStmt()
+            : isa<CaseStmt>(*First) ? cast<CaseStmt>(*First)->getSubStmt()
+                                    : cast<DefaultStmt>(*First)->getSubStmt();
+        if (auto *SubIf = dyn_cast<IfStmt>(SubStmt)) {
+          if (!SubIf->getElse()) {
+            auto ThenReturnBool =
+                checkSingleStatement(SubIf->getThen(), parseReturnLiteralBool);
+            if (ThenReturnBool.getPointer() &&
+                ThenReturnBool.getInt() != RetStmt.getInt()) {
+              Check->replaceCompoundReturnWithCondition(
+                  Context, cast<ReturnStmt>(*Second), RetStmt.getInt(), SubIf);
+            }
+          }
+        }
+      }
+    }
     return true;
   }
 
 private:
   SimplifyBooleanExprCheck *Check;
-  const MatchFinder::MatchResult &Result;
+  ASTContext &Context;
 };
 
 SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", false)),
       ChainedConditionalAssignment(
           Options.get("ChainedConditionalAssignment", false)) {}
 
 static bool containsBoolLiteral(const Expr *E) {
   if (!E)
     return false;
   E = E->IgnoreParenImpCasts();
   if (isa<CXXBoolLiteralExpr>(E))
     return true;
   if (const auto *BinOp = dyn_cast<BinaryOperator>(E))
     return containsBoolLiteral(BinOp->getLHS()) ||
            containsBoolLiteral(BinOp->getRHS());
   if (const auto *UnaryOp = dyn_cast<UnaryOperator>(E))
     return containsBoolLiteral(UnaryOp->getSubExpr());
   return false;
 }
 
-void SimplifyBooleanExprCheck::reportBinOp(
-    const MatchFinder::MatchResult &Result, const BinaryOperator *Op) {
+void SimplifyBooleanExprCheck::reportBinOp(const ASTContext &Context,
+                                           const BinaryOperator *Op) {
   const auto *LHS = Op->getLHS()->IgnoreParenImpCasts();
   const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
   const CXXBoolLiteralExpr *Bool;
   const Expr *Other;
   if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)) != nullptr)
     Other = RHS;
   else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)) != nullptr)
     Other = LHS;
   else
     return;
 
   if (Bool->getBeginLoc().isMacroID())
     return;
 
   // FIXME: why do we need this?
   if (!isa<CXXBoolLiteralExpr>(Other) && containsBoolLiteral(Other))
     return;
 
   bool BoolValue = Bool->getValue();
 
-  auto ReplaceWithExpression = [this, &Result, LHS, RHS,
+  auto ReplaceWithExpression = [this, &Context, LHS, RHS,
                                 Bool](const Expr *ReplaceWith, bool Negated) {
     std::string Replacement =
-        replacementExpression(Result, Negated, ReplaceWith);
+        replacementExpression(Context, Negated, ReplaceWith);
     SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc());
-    issueDiag(Result, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range,
+    issueDiag(Context, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range,
               Replacement);
   };
 
   switch (Op->getOpcode()) {
   case BO_LAnd:
     if (BoolValue)
       // expr && true -> expr
       ReplaceWithExpression(Other, /*Negated=*/false);
case BO_NE:
     // expr != true -> !expr, expr != false -> expr
     ReplaceWithExpression(Other, /*Negated=*/BoolValue);
     break;
   default:
     break;
   }
 }
 
-void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
-                                                  bool Value,
-                                                  StringRef BooleanId) {
-  Finder->addMatcher(
-      ifStmt(hasCondition(literalOrNegatedBool(Value).bind(BooleanId)))
-          .bind(IfStmtId),
-      this);
-}
-
-void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
-                                                  bool Value, StringRef Id) {
-  Finder->addMatcher(
-      conditionalOperator(hasTrueExpression(literalOrNegatedBool(Value)),
-                          hasFalseExpression(literalOrNegatedBool(!Value)))
-          .bind(Id),
-      this);
-}
-
-void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
-                                                  bool Value, StringRef Id) {
-  if (ChainedConditionalReturn)
-    Finder->addMatcher(ifStmt(hasThen(returnsBool(Value, ThenLiteralId)),
-                              hasElse(returnsBool(!Value)))
-                           .bind(Id),
-                       this);
-  else
-    Finder->addMatcher(ifStmt(unless(hasParent(ifStmt())),
-                              hasThen(returnsBool(Value, ThenLiteralId)),
-                              hasElse(returnsBool(!Value)))
-                           .bind(Id),
-                       this);
-}
-
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   auto VarAssign = declRefExpr(hasDeclaration(decl().bind(IfAssignVarId)));
   auto VarRef = declRefExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
   auto MemAssign = memberExpr(hasDeclaration(decl().bind(IfAssignVarId)));
   auto MemRef = memberExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
   auto SimpleThen =
       binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarAssign, MemAssign)),
if (ChainedConditionalAssignment)
     Finder->addMatcher(ifStmt(hasThen(Then), hasElse(Else)).bind(Id), this);
   else
     Finder->addMatcher(
         ifStmt(unless(hasParent(ifStmt())), hasThen(Then), hasElse(Else))
             .bind(Id),
         this);
 }
 
-static internal::Matcher<Stmt> ifReturnValue(bool Value) {
-  return ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))
-      .bind(CompoundIfId);
-}
-
-static internal::Matcher<Stmt> returnNotValue(bool Value) {
-  return returnStmt(has(literalOrNegatedBool(!Value))).bind(CompoundReturnId);
-}
-
-void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
-                                                          bool Value,
-                                                          StringRef Id) {
-  if (ChainedConditionalReturn)
-    Finder->addMatcher(
-        compoundStmt(hasSubstatementSequence(ifReturnValue(Value),
-                                             returnNotValue(Value)))
-            .bind(Id),
-        this);
-  else
-    Finder->addMatcher(
-        compoundStmt(hasSubstatementSequence(ifStmt(hasThen(returnsBool(Value)),
-                                                    unless(hasElse(stmt())),
-                                                    unless(hasParent(ifStmt())))
-                                                 .bind(CompoundIfId),
-                                             returnNotValue(Value)))
-            .bind(Id),
-        this);
-}
-
-void SimplifyBooleanExprCheck::matchCaseIfReturnsBool(MatchFinder *Finder,
-                                                      bool Value,
-                                                      StringRef Id) {
-  internal::Matcher<Stmt> CaseStmt =
-      caseStmt(hasSubstatement(ifReturnValue(Value))).bind(CaseId);
-  internal::Matcher<Stmt> CompoundStmt =
-      compoundStmt(hasSubstatementSequence(CaseStmt, returnNotValue(Value)))
-          .bind(Id);
-  Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
-}
-
-void SimplifyBooleanExprCheck::matchDefaultIfReturnsBool(MatchFinder *Finder,
-                                                         bool Value,
-                                                         StringRef Id) {
-  internal::Matcher<Stmt> DefaultStmt =
-      defaultStmt(hasSubstatement(ifReturnValue(Value))).bind(DefaultId);
-  internal::Matcher<Stmt> CompoundStmt =
-      compoundStmt(hasSubstatementSequence(DefaultStmt, returnNotValue(Value)))
-          .bind(Id);
-  Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
-}
-
-void SimplifyBooleanExprCheck::matchLabelIfReturnsBool(MatchFinder *Finder,
-                                                       bool Value,
-                                                       StringRef Id) {
-  internal::Matcher<Stmt> LabelStmt =
-      labelStmt(hasSubstatement(ifReturnValue(Value))).bind(LabelId);
-  internal::Matcher<Stmt> CompoundStmt =
-      compoundStmt(hasSubstatementSequence(LabelStmt, returnNotValue(Value)))
-          .bind(Id);
-  Finder->addMatcher(CompoundStmt, this);
-}
-
 void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
   Options.store(Opts, "ChainedConditionalAssignment",
                 ChainedConditionalAssignment);
 }
 
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(translationUnitDecl().bind("top"), this);
 
-  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);
-
-  matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
-  matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
-
-  matchCaseIfReturnsBool(Finder, true, CaseCompoundBoolId);
-  matchCaseIfReturnsBool(Finder, false, CaseCompoundNotBoolId);
-
-  matchDefaultIfReturnsBool(Finder, true, DefaultCompoundBoolId);
-  matchDefaultIfReturnsBool(Finder, false, DefaultCompoundNotBoolId);
-
-  matchLabelIfReturnsBool(Finder, true, LabelCompoundBoolId);
-  matchLabelIfReturnsBool(Finder, false, LabelCompoundNotBoolId);
 }
 
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   if (Result.Nodes.getNodeAs<TranslationUnitDecl>("top"))
-    Visitor(this, Result).TraverseAST(*Result.Context);
-  else if (const Expr *TrueConditionRemoved =
-               getBoolLiteral(Result, ConditionThenStmtId))
-    replaceWithThenStatement(Result, TrueConditionRemoved);
-  else if (const Expr *FalseConditionRemoved =
-               getBoolLiteral(Result, ConditionElseStmtId))
-    replaceWithElseStatement(Result, FalseConditionRemoved);
-  else if (const auto *Ternary =
-               Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
-    replaceWithCondition(Result, Ternary, false);
-  else if (const auto *TernaryNegated =
-               Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
-    replaceWithCondition(Result, TernaryNegated, true);
-  else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
-    replaceWithReturnCondition(Result, If, false);
-  else if (const auto *IfNot =
-               Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
-    replaceWithReturnCondition(Result, IfNot, true);
+    Visitor(this, *Result.Context).traverse();
   else if (const auto *IfAssign =
                Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
     replaceWithAssignment(Result, IfAssign, false);
   else if (const auto *IfAssignNot =
                Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId))
     replaceWithAssignment(Result, IfAssignNot, true);
-  else if (const auto *Compound =
-               Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId))
-    replaceCompoundReturnWithCondition(Result, Compound, false);
-  else if (const auto *CompoundNot =
-               Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
-    replaceCompoundReturnWithCondition(Result, CompoundNot, true);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundBoolId))
-    replaceCaseCompoundReturnWithCondition(Result, false);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundNotBoolId))
-    replaceCaseCompoundReturnWithCondition(Result, true);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundBoolId))
-    replaceDefaultCompoundReturnWithCondition(Result, false);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundNotBoolId))
-    replaceDefaultCompoundReturnWithCondition(Result, true);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundBoolId))
-    replaceLabelCompoundReturnWithCondition(Result, false);
-  else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundNotBoolId))
-    replaceLabelCompoundReturnWithCondition(Result, true);
-  else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
-    Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
 }
 
-void SimplifyBooleanExprCheck::issueDiag(const MatchFinder::MatchResult &Result,
+void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
                                          SourceLocation Loc,
                                          StringRef Description,
                                          SourceRange ReplacementRange,
                                          StringRef Replacement) {
   CharSourceRange CharRange =
       Lexer::makeFileCharRange(CharSourceRange::getTokenRange(ReplacementRange),
-                               *Result.SourceManager, getLangOpts());
+                               Context.getSourceManager(), getLangOpts());
 
   DiagnosticBuilder Diag = diag(Loc, Description);
-  if (!containsDiscardedTokens(Result, CharRange))
+  if (!containsDiscardedTokens(Context, CharRange))
     Diag << FixItHint::CreateReplacement(CharRange, Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceWithThenStatement(
-    const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
-  const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
-  issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+    const ASTContext &Context, const IfStmt *IfStatement,
+    const Expr *BoolLiteral) {
+  issueDiag(Context, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
             IfStatement->getSourceRange(),
-            getText(Result, *IfStatement->getThen()));
+            getText(Context, *IfStatement->getThen()));
 }
 
 void SimplifyBooleanExprCheck::replaceWithElseStatement(
-    const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
-  const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
+    const ASTContext &Context, const IfStmt *IfStatement,
+    const Expr *BoolLiteral) {
   const Stmt *ElseStatement = IfStatement->getElse();
-  issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+  issueDiag(Context, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
             IfStatement->getSourceRange(),
-            ElseStatement ? getText(Result, *ElseStatement) : "");
+            ElseStatement ? getText(Context, *ElseStatement) : "");
 }
 
 void SimplifyBooleanExprCheck::replaceWithCondition(
-    const MatchFinder::MatchResult &Result, const ConditionalOperator *Ternary,
+    const ASTContext &Context, const ConditionalOperator *Ternary,
     bool Negated) {
   std::string Replacement =
-      replacementExpression(Result, Negated, Ternary->getCond());
-  issueDiag(Result, Ternary->getTrueExpr()->getBeginLoc(),
+      replacementExpression(Context, Negated, Ternary->getCond());
+  issueDiag(Context, Ternary->getTrueExpr()->getBeginLoc(),
             "redundant boolean literal in ternary expression result",
             Ternary->getSourceRange(), Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceWithReturnCondition(
-    const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
+    const ASTContext &Context, const IfStmt *If, const Expr *BoolLiteral,
+    bool Negated) {
   StringRef Terminator = isa<CompoundStmt>(If->getElse()) ? ";" : "";
-  std::string Condition = replacementExpression(Result, Negated, If->getCond());
+  std::string Condition =
+      replacementExpression(Context, Negated, If->getCond());
   std::string Replacement = ("return " + Condition + Terminator).str();
-  SourceLocation Start =
-      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getBeginLoc();
-  issueDiag(Result, Start, SimplifyConditionalReturnDiagnostic,
+  SourceLocation Start = BoolLiteral->getBeginLoc();
+  issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
             If->getSourceRange(), Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
-    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
-    bool Negated) {
-  const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
+    const ASTContext &Context, const CompoundStmt *Compound,
+    const ReturnStmt *Ret, bool Negated) {
 
   // Scan through the CompoundStmt to look for a chained-if construct.
   const IfStmt *BeforeIf = nullptr;
   CompoundStmt::const_body_iterator Current = Compound->body_begin();
   CompoundStmt::const_body_iterator After = Compound->body_begin();
   for (++After; After != Compound->body_end() && *Current != Ret;
        ++Current, ++After) {
     if (const auto *If = dyn_cast<IfStmt>(*Current)) {
       if (const Expr *Lit = stmtReturnsBool(If, Negated)) {
         if (*After == Ret) {
           if (!ChainedConditionalReturn && BeforeIf)
             continue;
 
           std::string Replacement =
-              "return " + replacementExpression(Result, Negated, If->getCond());
+              "return " +
+              replacementExpression(Context, Negated, If->getCond());
           issueDiag(
-              Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+              Context, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
               SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
           return;
         }
 
         BeforeIf = If;
       }
     } else {
       BeforeIf = nullptr;
     }
   }
 }
 
 void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
-    const MatchFinder::MatchResult &Result, bool Negated, const IfStmt *If) {
+    const ASTContext &Context, const ReturnStmt *Ret, bool Negated,
+    const IfStmt *If) {
   const auto *Lit = stmtReturnsBool(If, Negated);
-  const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
   const std::string Replacement =
-      "return " + replacementExpression(Result, Negated, If->getCond());
-  issueDiag(Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+      "return " + replacementExpression(Context, Negated, If->getCond());
+  issueDiag(Context, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
             SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
 }
 
-void SimplifyBooleanExprCheck::replaceCaseCompoundReturnWithCondition(
-    const MatchFinder::MatchResult &Result, bool Negated) {
-  const auto *CaseDefault = Result.Nodes.getNodeAs<CaseStmt>(CaseId);
-  const auto *If = cast<IfStmt>(CaseDefault->getSubStmt());
-  replaceCompoundReturnWithCondition(Result, Negated, If);
-}
-
-void SimplifyBooleanExprCheck::replaceDefaultCompoundReturnWithCondition(
-    const MatchFinder::MatchResult &Result, bool Negated) {
-  const SwitchCase *CaseDefault =
-      Result.Nodes.getNodeAs<DefaultStmt>(DefaultId);
-  const auto *If = cast<IfStmt>(CaseDefault->getSubStmt());
-  replaceCompoundReturnWithCondition(Result, Negated, If);
-}
-
-void SimplifyBooleanExprCheck::replaceLabelCompoundReturnWithCondition(
-    const MatchFinder::MatchResult &Result, bool Negated) {
-  const auto *Label = Result.Nodes.getNodeAs<LabelStmt>(LabelId);
-  const auto *If = cast<IfStmt>(Label->getSubStmt());
-  replaceCompoundReturnWithCondition(Result, Negated, If);
-}
-
 void SimplifyBooleanExprCheck::replaceWithAssignment(
     const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
     bool Negated) {
   SourceRange Range = IfAssign->getSourceRange();
-  StringRef VariableName =
-      getText(Result, *Result.Nodes.getNodeAs<Expr>(IfAssignVariableId));
+  StringRef VariableName = getText(
+      *Result.Context, *Result.Nodes.getNodeAs<Expr>(IfAssignVariableId));
   StringRef Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =
-      replacementExpression(Result, Negated, IfAssign->getCond());
+      replacementExpression(*Result.Context, Negated, IfAssign->getCond());
   std::string Replacement =
       (VariableName + " = " + Condition + Terminator).str();
   SourceLocation Location =
       Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(IfAssignLocId)->getBeginLoc();
-  issueDiag(Result, Location,
+  issueDiag(*Result.Context, Location,
             "redundant boolean literal in conditional assignment", Range,
             Replacement);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang