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

[Alexander Kornienko]

================
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/