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

[Alexander Kornienko]

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


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:111
@@ -110,4 +110,3 @@
         return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
-                " " + getText(Result, *BinOp->getRHS()))
-            .str();
+                " " + getText(Result, *BinOp->getRHS())).str();
       }
----------------
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).

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:121
@@ +120,3 @@
+
+bool get(const OptionsView &Options, StringRef LocalName) {
+  return Options.get(LocalName, 0U) != 0U;
----------------
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).

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:211
@@ -201,5 +210,3 @@
                                                   bool Value, StringRef Id) {
-  Finder->addMatcher(ifStmt(isExpansionInMainFile(),
-                            hasThen(ReturnsBool(Value, ThenLiteralId)),
-                            hasElse(ReturnsBool(!Value))).bind(Id),
-                     this);
+  if (ChainedConditionalReturn) {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
----------------
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:41
@@ +40,3 @@
+///
+/// When a conditional boolean return appears at the end of a chain of `if`,
+/// `else if` statements, the conditional statement is left unchanged unless
----------------
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:52
@@ +51,3 @@
+  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+  virtual ~SimplifyBooleanExprCheck() {}
+
----------------
Please remove the empty destructor, the top base class has a virtual destructor (we need a check for this as well ;).

http://reviews.llvm.org/D8996

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/