================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:141
@@ +140,3 @@
+
+bool stmtReturnsBool(IfStmt *IfRet, bool Negated,
+ const CXXBoolLiteralExpr *&Lit) {
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Instead of using the return value and additionally return a literal by
> > reference, I'd suggest making the function return `llvm::Optional<const
> > CXXBoolLiteralExpr*>`.
> >
> > Same above.
> Any reason to not return just the pointer and compare against `nulltpr`?
If it's enough, sure.
================
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, ">="),
----------------
Can we use braced initialization instead of `std::make_pair` (`{{OO_EqualEqual,
"=="}, ....}`)? Same below.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:139
@@ +138,3 @@
+StringRef asBool(StringRef text, bool NeedsStaticCast) {
+ if (NeedsStaticCast) {
+ return ("static_cast<bool>(" + text + ")").str();
----------------
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.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:140
@@ +139,3 @@
+ if (NeedsStaticCast) {
+ return ("static_cast<bool>(" + text + ")").str();
+ }
----------------
This will return a StringRef pointing to a memory consumed by a temporary
string. Make the return type std::string to avoid the problem.
================
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) {
----------------
No `else` after `return`, please.
================
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) {
----------------
Can the body be empty? If not, I'd document this (e.g. using an assert()).
================
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.
----------------
nit: parentheses? I didn't find what "parenthesese" is.
================
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
----------------
ditto
================
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
----------------
Did you intentionally skip a number for this paragraph? Sam below after 3.
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
