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

[Richard]

In http://reviews.llvm.org/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.)


================
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();
       }
----------------
alexfh wrote:
> 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).
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.

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

================
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(),
----------------
alexfh wrote:
> 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.
I thought about that, but I didn't try it out.  I can go either way.

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

http://reviews.llvm.org/D8996

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