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

[Alexander Kornienko]

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:139
@@ +138,3 @@
+StringRef asBool(StringRef text, bool NeedsStaticCast) {
+  if (NeedsStaticCast) {
+    return ("static_cast<bool>(" + text + ")").str();
----------------
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.

================
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) {
----------------
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?

http://reviews.llvm.org/D9810

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