[PATCH] Enhance clang-tidy readability-simplify-boolean-expr to handle 'if (e) return true; return false; ' and improve replacement expressions.

[Richard]

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:109
@@ +108,3 @@
+std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
+    std::make_pair(OO_EqualEqual, "=="), std::make_pair(OO_ExclaimEqual, "!="),
+    std::make_pair(OO_Less, "<"),        std::make_pair(OO_GreaterEqual, ">="),
----------------
alexfh wrote:
> Can we use braced initialization instead of `std::make_pair` (`{{OO_EqualEqual, "=="}, ....}`)? Same below.
Good idea.  Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:139
@@ +138,3 @@
+StringRef asBool(StringRef text, bool NeedsStaticCast) {
+  if (NeedsStaticCast) {
+    return ("static_cast<bool>(" + text + ")").str();
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > In clang-tidy code (and in LLVM in general) it's more common to omit braces around single-line `if` bodies. One exception is when any branch of the if-else chain uses braces, we put them to all branches.
> > > 
> > >  Please fix here and in other places in this CL.
> > [[ https://www.securecoding.cert.org/confluence/display/c/EXP19-C.+Use+braces+for+the+body+of+an+if,+for,+or+while+statement | CERT coding standards require braces ]] around all control structures, so I've routinely adopted that habit.  Considering that existing security vulnerabilities were traced to the omission of the braces, I don't simply consider it a matter of style.
> > 
> > However, I'm not in charge of llvm code and if it is **really** that important, I can change it.
> > 
> > Sometimes these code reviews are simply exhausting because of all this little fiddling about with formatting instead of function.
> > CERT coding standards require braces around all control structures, so 
> > I've routinely adopted that habit. Considering that existing security 
> > vulnerabilities were traced to the omission of the braces, I don't simply 
> > consider it a matter of style.
> 
> This concern doesn't apply to the code that is routinely automatically formatted, as there's little to no chance of a line with incorrect indentation being considered a part of a branch when it's actually outside.
> 
> > ... if it is really that important ...
> 
> Consistency and readability are *really* that important. IMO, braces around single-line bodies of conditions/loops don't hurt readability as long as they are used consistently. Given that the code in this file is going to be modified mostly by you, I'm fine with putting braces around single-line if/for/while bodies as long as you commit to follow this style consistently in the files authored by you (and only there).
> 
> > Sometimes these code reviews are simply exhausting because of all this 
> > little fiddling about with formatting instead of function.
> 
> Luckily, we have clang-format that takes care of the most of formatting. And  what's left is not "little fiddling about with formatting" in most cases, but something more important. For example, here we're talking about a style preference "existing security vulnerabilities were traced to" in your own words. Also, I think, most style rules should be easy to internalize, at which point they won't require any special attention.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:140
@@ +139,3 @@
+  if (NeedsStaticCast) {
+    return ("static_cast<bool>(" + text + ")").str();
+  }
----------------
alexfh wrote:
> This will return a StringRef pointing to a memory consumed by a temporary string. Make the return type std::string to avoid the problem.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:252
@@ +251,3 @@
+    return stmtReturnsBool(Ret, Negated);
+  } else if (const auto *Compound = dyn_cast<CompoundStmt>(IfRet->getThen())) {
+    if (Compound->size() == 1) {
----------------
alexfh wrote:
> No `else` after `return`, please. 
FIxed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:554
@@ +553,3 @@
+  CompoundStmt::const_body_iterator After = Compound->body_begin();
+  for (++After; After != Compound->body_end() && *Current != Ret;
+       ++Current, ++After) {
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > Can the body be empty? If not, I'd document this (e.g. using an assert()).
> > The body can't be empty because the matcher ensures that it must contain at least two statements:
> > 
> >   - A `return` statement returning a boolean literal `false` or `true`
> >   - An `if` statement with no `else` clause and a then clause that consists of a single `return` statement returning the opposite boolean literal `true` or `false`
> > 
> > 
> Thanks for the explanation! Would you mind putting an `assert` then?
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:41
@@ +40,3 @@
+/// The resulting expression `e` is modified as follows:
+/// 1. Unnecessary parenthesese around the expression are removed.
+/// 2. Negated applications of `!` are eliminated.
----------------
alexfh wrote:
> nit: parentheses? I didn't find what "parenthesese" is.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:53
@@ +52,3 @@
+/// 1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant
+/// parenthesese and becomes `bool b = i < 0;`.
+/// The ternary assignment `bool b = (i & 1) ? true : false;` has an implicit
----------------
alexfh wrote:
> ditto
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:54
@@ +53,3 @@
+/// parenthesese and becomes `bool b = i < 0;`.
+/// The ternary assignment `bool b = (i & 1) ? true : false;` has an implicit
+/// conversion of `i & 1` to `bool` and becomes
----------------
alexfh wrote:
> Did you intentionally skip a number for this paragraph? Sam below after 3.
This should have been associated with the paragraph giving examples of implicit conversion to `bool`.  Fixed.

http://reviews.llvm.org/D9810

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