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