[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:
> > > 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;
```



http://reviews.llvm.org/D16308