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/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
