Enhance clang-tidy readability-simplify-boolean-expr check to handle chained conditional assignment and chained conditional return

[LegalizeAdulthood]

Summary

Based on feedback from applying this tool to the clang/LLVM codebase, this changeset improves the readability-simplify-boolean-expr check so that conditional assignment or return statements at the end of a chain of if/else if statements are left unchanged unless a configuration option is supplied.

[LegalizeAdulthood]

Add documentation for ChainedConditionalAssignment option.

[alexfh]

Do we actually need two separate options here? Which combination of these would you use for LLVM code?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

The text on the left is clang-formatted. I prefer to keep code clang-format-clean, so if you don't like the result, please file a bug against clang-format rather than changing the formatting by hand (I personally don't care in this case).

121

I don't think this function makes code shorter or easier to understand. I'd remove it and use the implementation directly (maybe with implicit conversion to bool to avoid the == 0U part).

211

How about:

Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                        ChainedConditionalReturn ? anything() : unless(hasParent(ifStmt())),
                        hasThen(ReturnsBool(Value, ThenLiteralId)),
                        hasElse(ReturnsBool(!Value))).bind(Id),
                 this);

? (if this works, of course)

Same below.

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

I'd deduplicate some text here:

/// When a conditional boolean return (or assignment) appears at the end
/// of a chain of `if`, `else if` statements, the conditional statement is left
/// unchanged unless the option `ChainedConditionalReturn`
/// (`ChainedConditionalAssignmentis for an assignment) specified as non-zero.

52

Please remove the empty destructor, the top base class has a virtual destructor (we need a check for this as well ;).

[LegalizeAdulthood]

In D8996#155356, @alexfh wrote:

Do we actually need two separate options here? Which combination of these would you use for LLVM code?

Since this is a readability check and "beauty is in the eye of the beholder", I didn't want to make decisions for the user. This extension of the check embraces LLVM preferences by default based on feedback from the use of the check on the clang/LLVM code. (In particular, these two issues came up in the review of changes to clang/lib/Parse and clang/lib/AST.)

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

111

This change was introduced by clang-format from the latest. I don't apply any of my own personal taste in formatting anymore, but routinely run everything through clang-format before posting for review.

121

I meant to bring this up on the mailing list. I got stuck for a while because bool b = false; Options.get(LocalName, b); totally failed when LocalName was configured to true. I tried to dig into it and figure out why, apparently there is a member function template that is enable_if'ed for integral types and because bool is an integral type, it was trying to turn true into an integer and silently failing, leaving my boolean setting unset. I'd rather have the setting just plain work for booleans because it's weird to turn this yes/no option into 1/0 in the configuration. So I was trying to have a local function that did "what I wanted" from OptionView until I could figure out how to get OptionView to properly handle boolean options.

211

I thought about that, but I didn't try it out. I can go either way.

clang-tidy/readability/SimplifyBooleanExprCheck.h

52

OK. Most code bases where I've worked had explicit preferences for virtual d'tors to remind readers that the class participates in a virtual method hierarchy.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

121

Fixed.

211

I tried this and it gets a compile error because anything() and unless(hasParent(ifStmt())) don't have compatible types.

clang-tidy/readability/SimplifyBooleanExprCheck.h

41

Fixed.

52

Fixed.

[LegalizeAdulthood]

Update based on review comments.

[alexfh]

Looks good!

Sorry for the long delay. I was on vacation. Do you need me to commit this for you?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

212

I guess I missed this in the initial code review.

Using isExpansionInMainFile() you disable fixes in headers which is sub-optimal. I think we should disable fixes in macros (and when a true or false comes from a macro) in a different way: check the relevant source location for being a file location (SourceLocation::isFileID).

This is fine for a follow up.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

212

I will address this in a subsequent refinement.

[LegalizeAdulthood]

Add to CODE_OWNERS.TXT

[LegalizeAdulthood]

If this is acceptable, please commit for me, thanks :-).

[alexfh]

CODE_OWNERS.TXT

11 ↗

(On Diff #25922)

I don't think we add all contributors here. Also, "code author" and "code owner" are different concepts, the latter is defined at the top of this file. I doubt that each clang-tidy check needs a dedicated "owner" as per the definition above.

I'm not going to commit this particular change.

[LegalizeAdulthood]

CODE_OWNERS.TXT

11 ↗

(On Diff #25922)

Either way is fine with me. I was simply attempting to offload support requests from you to me :-).

[alexfh]

Committed revision 237541.

clang-tidy/readability/SimplifyBooleanExprCheck.h

 /// `e ? false : true`                         becomes `!e`
 /// `if (true) t(); else f();`                 becomes `t();`
 /// `if (false) t(); else f();`                becomes `f();`
-/// `if (e) return true; else return false;`   becomes `return (e);`
-/// `if (e) return false; else return true;`   becomes `return !(e);`
+/// `if (e) return true; else return false;`   becomes `return e;`
+/// `if (e) return false; else return true;`   becomes `return !e;`
 /// `if (e) b = true; else b = false;`         becomes `b = e;`
-/// `if (e) b = false; else b = true;`         becomes `b = !(e);`
+/// `if (e) b = false; else b = true;`         becomes `b = !e;`
+///
+/// Parenthesis from the resulting expression `e` are removed whenever
+/// possible.
+///
+/// When a conditional boolean return appears at the end of a chain of `if`,
+/// `else if` statements, the conditional statement is left unchanged unless
+/// the option `ChainedConditionalReturn` is specified as non-zero.
+///
+/// When a conditional boolean assignment appears at the end of a chain of `if`,
+/// `else if` statements, the conditional assignment is left unchanged unless
+/// the optoin `ChainedConditionalAssignment` is specified as non-zero.
 ///
 class SimplifyBooleanExprCheck : public ClangTidyCheck {
 public:
-  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+  virtual ~SimplifyBooleanExprCheck() {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 

   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
                         const IfStmt *If, bool Negated = false);
+
+  const bool ChainedConditionalReturn;
+  const bool ChainedConditionalAssignment;
 };
 
 } // namespace readability

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

       StringRef NegatedOperator = negatedOperator(BinOp);
       if (!NegatedOperator.empty()) {
         return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
-                " " + getText(Result, *BinOp->getRHS()))
-            .str();
+                " " + getText(Result, *BinOp->getRHS())).str();
       }
     }
   }
   StringRef Text = getText(Result, *E);
   return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
                                                       : "!" + Text)
-                  : Text)
-      .str();
+                  : Text).str();
+}
+
+bool get(const OptionsView &Options, StringRef LocalName) {
+  return Options.get(LocalName, 0U) != 0U;
 }
 
 } // namespace
 
+SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      ChainedConditionalReturn(get(Options, "ChainedConditionalReturn")),
+      ChainedConditionalAssignment(
+          get(Options, "ChainedConditionalAssignment")) {}
+
 void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef OperatorName,

 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
-  Finder->addMatcher(ifStmt(isExpansionInMainFile(),
-                            hasThen(ReturnsBool(Value, ThenLiteralId)),
-                            hasElse(ReturnsBool(!Value))).bind(Id),
-                     this);
+  if (ChainedConditionalReturn) {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              hasThen(ReturnsBool(Value, ThenLiteralId)),
+                              hasElse(ReturnsBool(!Value))).bind(Id),
+                       this);
+  } else {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())),
+                              hasThen(ReturnsBool(Value, ThenLiteralId)),
+                              hasElse(ReturnsBool(!Value))).bind(Id),
+                       this);
+  }
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,

       hasRHS(boolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
-  Finder->addMatcher(
-      ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
-      this);
+  if (ChainedConditionalAssignment) {
+    Finder->addMatcher(
+        ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
+        this);
+  } else {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())), hasThen(Then),
+                              hasElse(Else)).bind(Id),
+                       this);
+  }
+}
+
+void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
+  Options.store(Opts, "ChainedConditionalAssignment",
+                ChainedConditionalAssignment);
 }
 
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {

test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: 1}]}" --
+// REQUIRES: shell
+
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  if (i < 0) {
+    b = true;
+  } else if (i < 10) {
+    b = false;
+  } else if (i > 20) {
+    b = true;
+  } else {
+    b = false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    b = false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else b = i > 20;{{$}}
+}
+
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+  // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES:      {{^}}  else if (i < 10)
+  // CHECK-FIXES-NEXT: {{^}}    b = false;
+  // CHECK-FIXES-NEXT: {{^}}  else b = i > 20;
+}

test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: 1}]}" --
+// REQUIRES: shell
+
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    return false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else return i > 20;{{$}}
+}
+
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  else if (i > 20)
+    return true;
+  else
+    return false;
+  // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES:      {{^}}  else if (i < 10)
+  // CHECK-FIXES-NEXT: {{^}}    return false;
+  // CHECK-FIXES-NEXT: {{^}}  else return i > 20;
+}

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

   } else
     f = false;
 }
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  else if (i > 20)
+    return true;
+  else
+    return false;
+}
+
+// unchanged: chained assignments, but ChainedConditionalAssignment not set
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  if (i < 0) {
+    b = true;
+  } else if (i < 10) {
+    b = false;
+  } else if (i > 20) {
+    b = true;
+  } else {
+    b = false;
+  }
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+}