[clang-tidy] Recognize labelled statements when simplifying boolean exprs

[LegalizeAdulthood]

Summary

[clang-tidy] Recognize labelled statements when simplifying boolean exprs

Inside a switch the caseStmt() and defaultStmt() have a nested statement
associated with them. Similarly, labelStmt() has a nested statement.
These statements were being missed when looking for a compound-if of the
form "if (x) return true; return false;" when the if is nested under one
of these labelling constructs.

Enhance the matchers to look for these nested statements using some
private matcher hasSubstatement() traversal matcher on case, default
and label statements. Add the private matcher hasSubstatementSequence()
to match the compound "if (x) return true; return false;" pattern.

• Add unit tests for private matchers and corresponding test infrastructure
• Add corresponding test file readability-simplify-bool-expr-case.cpp.
• Fix variable name copy/paste error in readability-simplify-bool-expr.cpp.
• Drop the asserts, which were used only for debugging matchers.
• Run clang-format on the whole check.
• Move local functions out of anonymous namespace and declare state, per LLVM style guide
• Declare labels constexpr
• Declare visitor arguments as pointer to const
• Drop braces around simple control statements per LLVM style guide
• Prefer explicit arguments over default arguments to methods

Fixes #27078

[LegalizeAdulthood]

clang-format

[JonasToth]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

386

const on values is uncommon in clang-tidy code. Please keep that consistent.

743

double space

745

redundant empty comment line

747

I think this assertion does not hold true from the matcher.
The matcher requires only hasDescendent(ifStmt()), but that does not ensure the first stmt is the ifStmt.

e.g.

case 10: {
  loggingCall();
  if(foo) ...

Is this excluded?
}

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

511

binding -> Binding

529

binding -> Binding

Also, there is no "case statement" involved here. ;-)

533–540

The check duplication here is unfortunate -- can you factor out the hasDescendant() bits into a variable that is reused, and perhaps use anyOf(caseStmt(stuff()), defaultStmt(stuff())) rather than separate functions?

718

Don't use an identifier with the same name as a type, please.

719

Add a message to the assertion (same with the other ones).

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

386

I can change the code, but I don't understand the push back.

"That's the way it's done elsewhere" just doesn't seem like good reasoning.

I write const on values to signify that they are computed once and only once. What is gained by removing that statement of once-and-only-once?

533–540

I'm not a fan of duplication, either.

However, I have to know if it's a CaseStmt or DefaultStmt in the replacement code and that's tied to the id, so I'm not sure how to collapse it further.

719

I'm not sure what you're asking for. I based these asserts off the existing assert in the file.

743

What exactly is the problem?

745

Meh, it's not redundant. It's there to aid readability of a big block of text by visually separating it from the associated code.

Why is this a problem?

747

Look more carefully at the AST. CaseStmt has exactly one child. That child can either be a compound statement block (which was already correctly handled by the check) or it can be a single statement. This change enhances the check to handle the single statement child of the CaseStmt and DefaultStmt.

[riccibruno]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

719

Something like assert((SwitchCase != nullptr) && "Some appropriate message blablabla!");
Keep the parentheses around the operands of != even if they are not strictly needed
since otherwise some bots will complain.

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

719

OK, I can do that; I wasn't sure if it was being suggested that I use some custom assert macro that took the message as a parameter.

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

No really, update from review comments :)

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

386

"That's the way it's done elsewhere" just doesn't seem like good reasoning.

Keeping the code consistent with the vast majority of the remainder of the project is valid reasoning. I am echoing the request to drop the top-level const. We don't use this pattern for non-pointer/reference types and there's not a whole lot gained by using it inconsistently.

533–540

You can bind to the castStmt() and defaultStmt() matchers to see what you get back, no? e.g., anyOf(caseStmt(stuff()).bind("which"), defaultStmt(stuff()).bind("which")) and in the check, you can use isa<> on the node named "which" to see whether you matched the case or the default label.

[JonasToth]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

386

Plus we do have a clang-tidy check (in the pipeline) that could do that automatically if the LLVM projects decides to go that route. So we won't suffer in the future, if we add the const.

745

Its subjective, I wouldn't do it so I thought you might have overlooked it, if we prefer this its fine from my side.

[alexfh]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

517

Please put the more expensive matcher (hasDescendant may be *really* expensive) last. Same below.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

386

You haven't addressed my point, which is that const is for values that don't change. Instead, you're just repeating "we haven't done it that way" instead of refuting the utility of const.

[LegalizeAdulthood]

Revive this patch on top-of-tree

[njames93]

A large portion of the changes, especially in the checks implementation file, appear to be NFC stylistic or formatting only fixes. While these changes are generally good, they shouldn't be a part of this patch. Instead they should be committed in their own NFC patch.
This makes it much easier to review the relevant changes needed to implement this new behaviour.

[LegalizeAdulthood]

In D56303#3266252, @njames93 wrote:

A large portion of the changes, especially in the checks implementation file, appear to be NFC stylistic or formatting only fixes. While these changes are generally good, they shouldn't be a part of this patch. Instead they should be committed in their own NFC patch.
This makes it much easier to review the relevant changes needed to implement this new behaviour.

It's going to be a significant amount of work to tease everything
apart and this patch has already been waiting literally for years.
Every time I put something up for review I get told "do this because
the style guide says so", so I anticipated those things and fixed them
pre-emptively.

Honestly, we need to make our review process more productive,
not less.

[LegalizeAdulthood]

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

72–73 ↗

(On Diff #402370)

FYI

These tabs here (and elsewhere) are phantoms somehow
introduced into the diff by Phabricator. My local diff file
that I uploaded has zero tabs in it anywhere.

[LegalizeAdulthood]

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

72–73 ↗

(On Diff #402370)

When I look more carefully, I see it's one column to the left of the
first column in the file.... so it's trying to somehow say that this line
had increased indent in the diff?

I thought it was a TAB indicator and another reviewer thought so as well.

But it isn't a TAB at all :)

[aaron.ballman]

In D56303#3270090, @LegalizeAdulthood wrote:

In D56303#3266252, @njames93 wrote:

A large portion of the changes, especially in the checks implementation file, appear to be NFC stylistic or formatting only fixes. While these changes are generally good, they shouldn't be a part of this patch. Instead they should be committed in their own NFC patch.
This makes it much easier to review the relevant changes needed to implement this new behaviour.

It's going to be a significant amount of work to tease everything
apart and this patch has already been waiting literally for years.

I don't think you need to tease it apart for this patch, but FWIW, I agree that this is much harder to review in this state.

Every time I put something up for review I get told "do this because
the style guide says so", so I anticipated those things and fixed them
pre-emptively.

The rule of thumb is: if you have significant NFC changes, do them first and push the changes (often doesn't even require a review because it's NFC, but that depends on what comfort level you have about the changes being NFC), and then do the functional changes on top of the improved code. Reviewers will ask you to update style for the lines of code you're touching when appropriate, but we (generally) shouldn't be asking you to fix style issues in code you're not touching. (Oddly, we touch on this in #2 at https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access which is not where I'd expect that information to live.)

FWIW, I looked through the changes and don't have major concerns. A second set of eyes approving this would be appreciated, but this LGTM.

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

72–73 ↗

(On Diff #402370)

Yeah, that's caught me a few times as well. The >> seems to mean "indentation changed" not "tab inserted".

422 ↗

(On Diff #402370)

I'd prefer the NOLINT comment be removed (we don't typically add those, same as clang-format comments or other special markings).

[LegalizeAdulthood]

In D56303#3272711, @aaron.ballman wrote:

The rule of thumb is: if you have significant NFC changes,
do them first and push the changes (often doesn't even
require a review because it's NFC, but that depends on
what comfort level you have about the changes being NFC),
and then do the functional changes on top of the improved
code.

This is reasonable, but is it written down anywhere so that people
don't have to through this trial by fire every time? :)

[LegalizeAdulthood]

Update from review comments

[aaron.ballman]

In D56303#3273694, @LegalizeAdulthood wrote:

In D56303#3272711, @aaron.ballman wrote:

The rule of thumb is: if you have significant NFC changes,
do them first and push the changes (often doesn't even
require a review because it's NFC, but that depends on
what comfort level you have about the changes being NFC),
and then do the functional changes on top of the improved
code.

This is reasonable, but is it written down anywhere so that people
don't have to through this trial by fire every time? :)

Yup, in about the most obtuse place you could image: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. We could probably stand to update https://llvm.org/docs/DeveloperPolicy.html to be more explicit about this and document it in a much more obvious place. (I agree with the complaint about us doing a poor job of writing these kinds of expectations down.)

Oops, forgot to accept the last time I said I thought this looked good. Please still give a day or two before landing in case @JonasToth or @alexfh have further comments or concerns.

[kwk]

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp

1 ↗

(On Diff #404179)

@LegalizeAdulthood I'm doing standalone builds of the LLVM projects and we're seeing an error that this file cannot be found. I wonder why this even worked before because we're essentially checking out (or better extract) clang into /clang-15.0.0-src/ and clang-extra-tools into clang-extra-tools-15.0.0-src. So no matter how smart you design your -I include path flags, there`ll never be a ../../clang that contains the file to be included here. @tstellar does this ring a bell? I've looked at this with @serge-sans-paille today and be both thought this cannot work. Here's the logfile

builder-live.log.gz242 KBDownload

of this build (which might be unreachable in a few days): https://download.copr.fedorainfracloud.org/results/@fedora-llvm-team/llvm-snapshots/fedora-35-x86_64/04639555-clang/builder-live.log.gz

[njames93]

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp

1 ↗

(On Diff #404179)

TBH I think this include, allTestClangConfigs below and the CMakeLists edits should have been removed in D125026. I'll put a patch up to address this.

clang-tidy/readability/SimplifyBooleanExprCheck.h

void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef Id);
 
   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
   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);
+                       const ConditionalOperator *Ternary, bool Negated);
 
   void replaceWithReturnCondition(
       const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,
-      bool Negated = false);
+      bool Negated);
 
   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
-                        const IfStmt *If, bool Negated = false);
+                        const IfStmt *If, bool Negated);
 
   void replaceCompoundReturnWithCondition(
       const ast_matchers::MatchFinder::MatchResult &Result,
-      const CompoundStmt *Compound, bool Negated = false);
+      const CompoundStmt *Compound, bool Negated);
+
+  void replaceSwitchCaseCompoundReturnWithCondition(
+      const ast_matchers::MatchFinder::MatchResult &Result,
+      const CompoundStmt *Compound, bool Negated, const SwitchCase *SwitchCase);
+
+  void replaceCaseCompoundReturnWithCondition(
+      const ast_matchers::MatchFinder::MatchResult &Result,
+      const CompoundStmt *Compound, bool Negated);
+
+  void replaceDefaultCompoundReturnWithCondition(
+      const ast_matchers::MatchFinder::MatchResult &Result,
+      const CompoundStmt *Compound, bool Negated);
 
   void issueDiag(const ast_matchers::MatchFinder::MatchResult &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-tidy/readability/SimplifyBooleanExprCheck.cpp

 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 CompoundReturnId[] = "compound-return";
+const char CompoundIfId[] = "compound-if";
 const char CompoundBoolId[] = "compound-bool";
 const char CompoundNotBoolId[] = "compound-bool-not";
+const char CaseId[] = "case";
+const char CaseCompoundBoolId[] = "case-compound-bool";
+const char CaseCompoundNotBoolId[] = "case-compound-bool-not";
+const char DefaultId[] = "default";
+const char DefaultCompoundBoolId[] = "default-compound-bool";
+const char DefaultCompoundNotBoolId[] = "default-compound-bool-not";
 
 const char IfStmtId[] = "if";
 
 const char SimplifyOperatorDiagnostic[] =
     "redundant boolean literal supplied to boolean operator";
 const char SimplifyConditionDiagnostic[] =
     "redundant boolean literal in if statement condition";
 const char SimplifyConditionalReturnDiagnostic[] =
bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
   }
 
   return false;
 }
 
 } // namespace
 
 class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
- public:
   Visitor(SimplifyBooleanExprCheck *Check,
           const MatchFinder::MatchResult &Result)
       : Check(Check), Result(Result) {}
 
   bool VisitBinaryOperator(BinaryOperator *Op) {
     Check->reportBinOp(Result, Op);
     return true;
   }
 
- private:
   SimplifyBooleanExprCheck *Check;
   const MatchFinder::MatchResult &Result;
 };
 
-
 SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)),
       ChainedConditionalAssignment(
           Options.get("ChainedConditionalAssignment", 0U)) {}
 
 bool containsBoolLiteral(const Expr *E) {
bool containsBoolLiteral(const Expr *E) {
   return false;
 }
 
 void SimplifyBooleanExprCheck::reportBinOp(
     const MatchFinder::MatchResult &Result, const BinaryOperator *Op) {
   const auto *LHS = Op->getLHS()->IgnoreParenImpCasts();
   const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
-  const CXXBoolLiteralExpr *Bool = nullptr;
-  const Expr *Other = nullptr;
+  const CXXBoolLiteralExpr *Bool;
+  const Expr *Other;
   if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)))
     Other = RHS;
   else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)))
     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();
+  const bool BoolValue = Bool->getValue();
 
-  auto replaceWithExpression = [this, &Result, LHS, RHS, Bool](
-                                   const Expr *ReplaceWith, bool Negated) {
-    std::string Replacement =
+  const auto replaceWithExpression =
+      [this, &Result, LHS, RHS, Bool](const Expr *ReplaceWith, bool Negated) {
+        const std::string Replacement =
             replacementExpression(Result, Negated, ReplaceWith);
         SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc());
-    issueDiag(Result, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range,
-              Replacement);
+        issueDiag(Result, Bool->getBeginLoc(), SimplifyOperatorDiagnostic,
+                  Range, Replacement);
       };
 
   switch (Op->getOpcode()) {
-    case BO_LAnd:
-      if (BoolValue) {
-        // expr && true -> expr
-        replaceWithExpression(Other, /*Negated=*/false);
-      } else {
-        // expr && false -> false
-        replaceWithExpression(Bool, /*Negated=*/false);
-      }
-      break;
-    case BO_LOr:
-      if (BoolValue) {
-        // expr || true -> true
-        replaceWithExpression(Bool, /*Negated=*/false);
-      } else {
-        // expr || false -> expr
-        replaceWithExpression(Other, /*Negated=*/false);
-      }
-      break;
-    case BO_EQ:
-      // expr == true -> expr, expr == false -> !expr
-      replaceWithExpression(Other, /*Negated=*/!BoolValue);
-      break;
-    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(isExpansionInMainFile(),
Finder->addMatcher(
               ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
           hasAnySubstatement(returnStmt(has(ignoringParenImpCasts(
                                             cxxBoolLiteral(equals(!Value)))))
                                  .bind(CompoundReturnId)))
           .bind(Id),
       this);
 }
 
+void SimplifyBooleanExprCheck::matchCaseIfReturnsBool(MatchFinder *Finder,
+                                                      bool Value,
+                                                      StringRef Id) {
+  // Binding to the returnStmt matched is pointless because we can't guarantee
+  // anything about the ordering of the return statement and the case statement.
+  // If only we had a statement sequence matcher...
+  Finder->addMatcher(
+      switchStmt(
+          has(compoundStmt(
+                  has(caseStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+                                                    unless(hasElse(stmt())))
+                                                 .bind(CompoundIfId)))
+                          .bind(CaseId)),
+                  has(returnStmt(has(cxxBoolLiteral(equals(!Value))))))
+                  .bind(Id))),
+      this);
+}
+
+void SimplifyBooleanExprCheck::matchDefaultIfReturnsBool(MatchFinder *Finder,
+                                                         bool Value,
+                                                         StringRef Id) {
+  // Binding to the returnStmt matched is pointless because we can't guarantee
+  // anything about the ordering of the return statement and the default statement.
+  // If only we had a statement sequence matcher...
+  Finder->addMatcher(
+      switchStmt(has(
+          compoundStmt(
+              has(defaultStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+                                                   unless(hasElse(stmt())))
+                                                .bind(CompoundIfId)))
+                      .bind(DefaultId)),
+              has(returnStmt(has(cxxBoolLiteral(equals(!Value))))))
+              .bind(Id))),
+      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);
 }
 
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   if (Result.Nodes.getNodeAs<TranslationUnitDecl>("top"))
     Visitor(this, Result).TraverseAST(*Result.Context);
   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<ConditionalOperator>(TernaryId))
-    replaceWithCondition(Result, Ternary);
+    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);
+    replaceWithReturnCondition(Result, If, false);
   else if (const auto *IfNot =
                Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
     replaceWithReturnCondition(Result, IfNot, true);
   else if (const auto *IfAssign =
                Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
-    replaceWithAssignment(Result, IfAssign);
+    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);
-  else if (const auto *Compound =
+    replaceCompoundReturnWithCondition(Result, Compound, false);
+  else if (const auto *CompoundNot =
                Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
-    replaceCompoundReturnWithCondition(Result, Compound, true);
+    replaceCompoundReturnWithCondition(Result, CompoundNot, true);
+  else if (const auto *CaseCompound =
+               Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundBoolId))
+    replaceCaseCompoundReturnWithCondition(Result, CaseCompound, false);
+  else if (const auto *CaseCompoundNot =
+               Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundNotBoolId))
+    replaceCaseCompoundReturnWithCondition(Result, CaseCompoundNot, true);
+  else if (const auto *DefaultCompound =
+               Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundBoolId))
+    replaceDefaultCompoundReturnWithCondition(Result, DefaultCompound, false);
+  else if (const auto *DefaultCompoundNot =
+               Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundNotBoolId))
+    replaceDefaultCompoundReturnWithCondition(Result, DefaultCompoundNot, true);
+  else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
+    Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
 }
 
 void SimplifyBooleanExprCheck::issueDiag(
     const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc,
     StringRef Description, SourceRange ReplacementRange,
     StringRef Replacement) {
   CharSourceRange CharRange =
       Lexer::makeFileCharRange(CharSourceRange::getTokenRange(ReplacementRange),
for (++After; After != Compound->body_end() && *Current != Ret;
        ++Current, ++After) {
     if (const auto *If = dyn_cast<IfStmt>(*Current)) {
       if (const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated)) {
         if (*After == Ret) {
           if (!ChainedConditionalReturn && BeforeIf)
             continue;
 
           const Expr *Condition = If->getCond();
-          std::string Replacement =
+          const std::string Replacement =
               "return " + replacementExpression(Result, Negated, Condition);
           issueDiag(
               Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
               SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
           return;
         }
 
         BeforeIf = If;
       }
     } else {
       BeforeIf = nullptr;
     }
   }
 }
 
+void SimplifyBooleanExprCheck::replaceSwitchCaseCompoundReturnWithCondition(
+    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+    bool Negated, const SwitchCase *CaseDefault) {
+  assert(CaseDefault != nullptr);
+
+  // The body shouldn't be empty because the matcher ensures that it must
+  // contain at least two statements:
+  // 1) A `return` statement returning a boolean literal `false` or `true`
+  // 2) An `if` statement with no `else` clause that consists of a single
+  //    `return` statement returning the opposite boolean literal `true` or
+  //    `false`.
+  assert(Compound->size() >= 2);
+
+  // We're looking for something like:
+  //
+  // switch (expr) {
+  // case 10:
+  //   if (cond) return false;
+  //   return true;
+  // }
+  //
+  // The matcher ensures that the case/if combination is already there,
+  // and that there is a return statement of this form somewhere in the
+  // compound statement associated with the switch contains a return
+  // statement of this form, but the return statement could be anywhere
+  // within the compound statement, so we have to manually check to see
+  // if the next statement after the if contains a return statement of
+  // the correct form.  Ideally we'd be able to express this with the
+  // matchers, but that is currently impossible.
+  //
+  const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+  assert(If != nullptr);
+  const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated);
+  assert(Lit != nullptr);
+
+  // Advance to the matched CaseStmt
+  CompoundStmt::const_body_iterator Current = Compound->body_begin();
+  while (Current != Compound->body_end() && *Current != CaseDefault)
+    ++Current;
+  assert(Current != Compound->body_end());
+
+  // Advance to the next statement, watch out for something like:
+  //
+  // switch (expr) {
+  // case 0:
+  //   return true;
+  //
+  // case 1:
+  //   if (cond) return false;
+  // }
+  //
+  ++Current;
+  if (Current == Compound->body_end())
+    return;
+
+  if (const auto *Ret = dyn_cast<ReturnStmt>(*Current)) {
+    if (stmtReturnsBool(Ret, !Negated)) {
+      const Expr *Condition = If->getCond();
+      const std::string Replacement =
+          "return " + replacementExpression(Result, Negated, Condition);
+      issueDiag(Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+                SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+    }
+  }
+}
+
+void SimplifyBooleanExprCheck::replaceCaseCompoundReturnWithCondition(
+    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+    bool Negated) {
+  replaceSwitchCaseCompoundReturnWithCondition(
+      Result, Compound, Negated, Result.Nodes.getNodeAs<CaseStmt>(CaseId));
+}
+
+void SimplifyBooleanExprCheck::replaceDefaultCompoundReturnWithCondition(
+    const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+    bool Negated) {
+  replaceSwitchCaseCompoundReturnWithCondition(
+      Result, Compound, Negated,
+      Result.Nodes.getNodeAs<DefaultStmt>(DefaultId));
+}
+
 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 Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =

test/clang-tidy/readability-simplify-bool-expr-case.cpp

+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    if (b == true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+    if (b == false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(!b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+    if (b && true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+    if (b && false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(false\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+    if (b || true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(true\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+    if (b || false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+    return i > 0 ? true : false;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+    return i > 0 ? false : true;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+    if (true)
+      j = 10;
+    else
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES:      {{j = 10;$}}
+    // CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+    if (false)
+      j = -20;
+    else
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+    if (j > 10)
+      return true;
+    else
+      return false;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+    if (j > 10)
+      return false;
+    else
+      return true;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+    if (j > 10)
+      b = true;
+    else
+      b = false;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j > 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+    if (j > 10)
+      b = false;
+    else
+      b = true;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j <= 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+    if (j > 10)
+      return true;
+    return false;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // FIXES: {{return j > 10;}}
+
+  case 15:
+    if (j > 10)
+      return false;
+    return true;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // FIXES: {{return j <= 10;}}
+
+  case 16:
+    if (j > 10)
+      return true;
+    return true;
+    return false;
+
+  case 17:
+    if (j > 10)
+      return false;
+    return false;
+    return true;
+
+  case 100: {
+    if (b == true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+    if (b == false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(!b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 102: {
+    if (b && true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 103: {
+    if (b && false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(false\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 104: {
+    if (b || true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(true\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+  }
+
+  case 105: {
+    if (b || false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 106: {
+    return i > 0 ? true : false;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i > 0;}}
+  }
+
+  case 107: {
+    return i > 0 ? false : true;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i <= 0;}}
+  }
+
+  case 108: {
+    if (true)
+      j = 10;
+    else
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES:      {{j = 10;$}}
+    // CHECK-FIXES-NEXT: {{break;$}}
+  }
+
+  case 109: {
+    if (false)
+      j = -20;
+    else
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+  }
+
+  case 110: {
+    if (j > 10)
+      return true;
+    else
+      return false;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j > 10;}}
+  }
+
+  case 111: {
+    if (j > 10)
+      return false;
+    else
+      return true;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j <= 10;}}
+  }
+
+  case 112: {
+    if (j > 10)
+      b = true;
+    else
+      b = false;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j > 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+  }
+
+  case 113: {
+    if (j > 10)
+      b = false;
+    else
+      b = true;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j <= 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+  }
+
+  case 114: {
+    if (j > 10)
+      return true;
+    return false;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // CHECK-FIXES: {{return j > 10;}}
+  }
+
+  case 115: {
+    if (j > 10)
+      return false;
+    return true;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // CHECK-FIXES: {{return j <= 10;}}
+  }
+
+  case 116: {
+    return false;
+    if (j > 10)
+      return true;
+  }
+
+  case 117: {
+    return true;
+    if (j > 10)
+      return false;
+  }
+  }
+
+  return j > 0;
+}
+
+bool default_stmt0(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b == true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+  return false;
+}
+
+bool default_stmt1(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b == false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(!b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+  return false;
+}
+
+bool default_stmt2(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b && true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+  return false;
+}
+
+bool default_stmt3(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b && false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(false\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+  return false;
+}
+
+bool default_stmt4(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b || true)
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(true\)}}
+    // CHECK-FIXES-NEXT: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+  }
+  return false;
+}
+
+bool default_stmt5(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (b || false)
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+    // CHECK-FIXES: {{if \(b\)}}
+    // CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+  return false;
+}
+
+bool default_stmt6(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    return i > 0 ? true : false;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i > 0;}}
+  }
+  return false;
+}
+
+bool default_stmt7(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    return i > 0 ? false : true;
+    // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+    // CHECK-FIXES: {{return i <= 0;}}
+  }
+  return false;
+}
+
+bool default_stmt8(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (true)
+      j = 10;
+    else
+      j = -20;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES:      {{j = 10;$}}
+    // CHECK-FIXES-NEXT: {{break;$}}
+  }
+  return false;
+}
+
+bool default_stmt9(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (false)
+      j = -20;
+    else
+      j = 10;
+    break;
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+    // CHECK-FIXES: {{j = 10;}}
+    // CHECK-FIXES-NEXT: {{break;}}
+  }
+  return false;
+}
+
+bool default_stmt10(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      return true;
+    else
+      return false;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j > 10;}}
+  }
+  return false;
+}
+
+bool default_stmt11(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      return false;
+    else
+      return true;
+    // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+    // CHECK-FIXES: {{return j <= 10;}}
+  }
+  return false;
+}
+
+bool default_stmt12(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      b = true;
+    else
+      b = false;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j > 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+  }
+  return false;
+}
+
+bool default_stmt13(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      b = false;
+    else
+      b = true;
+    return b;
+    // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+    // CHECK-FIXES: {{b = j <= 10;}}
+    // CHECK-FIXES-NEXT: {{return b;}}
+  }
+  return false;
+}
+
+bool default_stmt14(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      return true;
+    return false;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // FIXES: {{return j > 10;}}
+  }
+  return false;
+}
+
+bool default_stmt15(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      return false;
+    return true;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+    // FIXES: {{return j <= 10;}}
+  }
+  return false;
+}
+
+bool default_stmt16(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return false;
+
+  default:
+    if (j > 10)
+      return true;
+  }
+  return false;
+}
+
+bool default_stmt17(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+    return true;
+
+  default:
+    if (j > 10)
+      return false;
+  }
+  return false;
+}