[PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===========================================  ================
-/// Initial expression                           Result
-/// -------------------------------------------  ----------------
-/// `if (b == true)`                             `if (b)`
-/// `if (b == false)`                            `if (!b)`
-/// `if (b && true)`                             `if (b)`
-/// `if (b && false)`                            `if (false)`
-/// `if (b || true)`                             `if (true)`
-/// `if (b || false)`                            `if (b)`
-/// `e ? true : false`                           `e`
-/// `e ? false : true`                           `!e`
-/// `if (true) t(); else f();`                   `t();`
-/// `if (false) t(); else f();`                  `f();`
-/// `if (e) return true; else return false;`     `return e;`
-/// `if (e) return false; else return true;`     `return !e;`
-/// `if (e) b = true; else b = false;`           `b = e;`
-/// `if (e) b = false; else b = true;`           `b = !e;`
-/// `if (e) return true; return false;`          `return e;`
-/// `if (e) return false; return true;`          `return !e;`
-/// ===========================================  ================
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///      opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///      comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///      are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant
-///      parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///      implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///      `return i >= 0;`.
-///
-///      The conditional return `if (i != 0) return false; return true;` becomes
-///      `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///      implicit conversion of a pointer to `bool` and becomes
-///      `return p != nullptr;`.
-///
-///      The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///      implicit conversion of `i & 1` to `bool` and becomes
-///      `bool b = static_cast<bool>(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` has
-///      an implicit conversion of an integer quantity `i & 1` to `bool` and
-///      becomes `return static_cast<bool>(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` of
-///      `struct X`, the conditional return `if (x) return true; return false;`
-///      becomes `return static_cast<bool>(x);`
-///
-/// 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` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html
----------------
aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > I think I can agree with that, but I also think it depends a lot on what the purpose to the check is. If it's "improve readability regarding parens" + options that control when to remove or add, that makes sense to me. Otherwise, I think segregating the checks into one that removes (plus options) and one that adds (plus options) makes sense -- even if they perhaps use the same underlying heuristic engine and are simply surfaced as separate checks.
> > > > This check isn't at all about the readability of parens, so it doesn't make sense for this check to be concerned with it IMO.
> > > Agreed; trying to suss out what the appropriate design for this particular check is. I think I've talked myself into "don't touch parens here."
> > Currently in this patch, parens are added when the expression compared to `nullptr` or `0` is a binary operator.
> > 
> > I think that is the right thing to do here so we get:
> > 
> > ```
> > bool b = (i & 1) == 0;
> > ```
> > 
> > instead of
> > 
> > ```
> > bool b = i & 1 == 0;
> > ```
> > 
> And we've come full circle to my original suggestion. :-D Personally, I find it to be more readable with the parens than without. I'm just worried that we have this check adding parens and another check removing them. But that can be handled in the check that removes them instead of here. My statement about not touching parens was horribly unclear, and I am sorry for that. What my brain was telling my fingers to type was:  Don't worry about parens here; add them if they add clarity, don't add them if they don't add clarity. We can worry about parens categorically as part of another check.
Yes, I did update the patch per your earlier review comments.

I agree this is more readable and that a future check that generally removes unnecessary parens can handle it if people don't like it.

Because there is no "right" answer, I prefer to deploy the check and await feedback from users to know if adding the parens is widely considered bad.  If the general consensus is that adding the parens is bad, this check can be enhanced with an option to decide when to add them.  Prior to this change, it only added them when necessary to preserve semantics.

In trying this check on llvm code base I have seen one occasion where the check produced something like `!!(complicated expression)`, but I haven't figured that one out yet.  It's on my list of things to improve with this check.

Once this change is accepted, I have another improvement waiting in the wings to simplify things like `!(a < b)` to `(a >= b)`, etc.  There has been some discussion earlier about how negated combinations of boolean expressions should be handled.  The majority of the comments so far feel that applying DeMorgan's law generally makes for better readability, but again with readability checks not everyone agrees.

For now, I believe I have addressed all of Aaron's outstanding comments.


http://reviews.llvm.org/D16308