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