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

[Alexander Kornienko]

A few minor issues, otherwise looks good.

I'm going to fix the code and commit the patch for you.


================
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:
> > 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.
Thanks for fixing this, but I was talking only about *single-line* bodies. Luckily, the braces that shouldn't have been removed can easily be added back by clang-tidy itself:

  clang-tidy -fix -checks=readability-braces-around-statements \
    -config="{CheckOptions: [{key: 'readability-braces-around-statements.ShortStatementLines', value: 2}]}" \
    clang-tidy/readability/SimplifyBooleanExprCheck.cpp

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:52
@@ -50,1 +51,3 @@
 const char IfAssignObjId[] = "if-assign-obj";
+const char CompoundId[] = "compound";
+const char CompoundIfReturnsBoolId[] = "compound-if";
----------------
This variable and the one below are not used.

================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:604
@@ +603,3 @@
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_simple_if_return_negated(int i) {
----------------
nit: capitalization, trailing period

http://reviews.llvm.org/D9810

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