[PATCH] Add readability-simplify-boolean-expr check to clang-tidy

[Alexander Kornienko]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:20
@@ +19,3 @@
+
+StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
Is this function used only once? Maybe inline it then?

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:32
@@ +31,3 @@
+const auto LeftBooleanLiteralId = "bool-op-expr-yields-bool";
+const auto RightExpressionId = "bool-op-expr-yields-expr";
+const auto RightBooleanLiteralId = "expr-op-bool-yields-bool";
----------------
This form is rather exotic in Clang and LLVM code. I'd still prefer `const char X[] = "...";`.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+                                         const char *const Id) {
----------------
Here and in a few other places in this patch: please use `const X *` instead of `X const *` (and the same for references) for consistency with other clang-tidy code.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:54
@@ +53,3 @@
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+                                         const char *const Id) {
+  CXXBoolLiteralExpr const *Literal =
----------------
Please use `StringRef` instead of `const char*` wherever it makes sense.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:57
@@ +56,3 @@
+      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
+  return Literal &&
+                 Result.SourceManager->isMacroBodyExpansion(
----------------
nit: It looks better with parentheses and makes operator precedence clear:

    return (Literal &&
            Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
               ? nullptr
               : Literal;


================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:66
@@ +65,3 @@
+SimpleReturnsBool(bool Value, char const *const Id = nullptr) {
+  return returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
+}
----------------
How about putting this into the `ReturnsBool` function?

  auto SimpleReturnsBool =
      returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:236
@@ +235,3 @@
+
+void SimplifyBooleanExpr::replaceWithLeftBooleanLiteral(
+    const MatchFinder::MatchResult &Result,
----------------
Any luck replacing `replaceWith*` methods with a single one as Samuel suggested?

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:300
@@ +299,3 @@
+    const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
+  const auto Terminator =
+      (If->getElse()->getStmtClass() == Stmt::StmtClass::CompoundStmtClass)
----------------
We use `auto` when it's clear what's behind it or the alternative isn't better. The most frequent use of auto is when the type (or its important part) is spelled in the RHS of the initialization. It also frequently makes sense to use `auto` in range-based for loops.

Here I would clearly use `StringRef` instead.

The comment applies to other usages of auto in this file as well.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:301
@@ +300,3 @@
+  const auto Terminator =
+      (If->getElse()->getStmtClass() == Stmt::StmtClass::CompoundStmtClass)
+          ? ";"
----------------
  isa<CompoundStmt>(If->getElse()) ? ";" : "";

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:304
@@ +303,3 @@
+          : "";
+  const auto Replacement = "return " + StringRef(Negated ? "!" : "") + "(" +
+                           getText(Result, *If->getCond()) + ")" + Terminator;
----------------
And here the use of auto is simply dangerous: this creates a Twine object that is only supposed to be used as a temporary (because if your `getText()` returned `std::string`, the Twine object would point to a deleted memory region right after the end of the expression). Please use `std::string` instead.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:306
@@ +305,3 @@
+                           getText(Result, *If->getCond()) + ")" + Terminator;
+  const auto Start =
+      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
----------------
Also not the best use for auto. Please use `SourceLocation` instead.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.h:23-34
@@ +22,14 @@
+/// Examples:
+/// if (b == true) becomes if (b)
+/// if (b == false) becomes if (!b)
+/// if (b && true) becomes if (b)
+/// if (b && false) becomes if (false)
+/// if (b || true) becomes if (true)
+/// if (b || false) becomes if (b)
+/// e ? true : false becomes e
+/// e ? false : true becomes !e
+/// if (true) t(); else f(); becomes t();
+/// if (false) t(); else f(); becomes f();
+/// if (e) return true; else return false; becomes return (e);
+/// if (e) return false; else return true; becomes return !(e);
+///
----------------
Some beutification wouldn't harm, e.g.:
  ///   `if (b == true)`                           becomes `if (b)`
  ///   `if (b == false)`                          becomes `if (!b)`
  ///   `if (b && true)`                           becomes `if (b)`
  ///   `if (b && false)`                          becomes `if (false)`
  ///   `if (b || true)`                           becomes `if (true)`
  ///   `if (b || false)`                          becomes `if (b)`
  ///   `e ? true : false`                         becomes `e`
  ///   `e ? false : true`                         becomes `!e`
  ///   `if (true) t(); else f();`                 becomes `t();`
  ///   `if (false) t(); else f();`                becomes `f();`
  ///   `if (e) return true; else return false;`   becomes `return (e);`
  ///   `if (e) return false; else return true;`   becomes `return !(e);`


================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:62
@@ +61,3 @@
+
+void if_with_bool_literal_condition()
+{
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Please clang-format the test with LLVM style and infinite column limit (to avoid breaking the CHECK lines).
> Many of the tests are specifically designed to show that the user's whitespace is unmolested.
> 
> If I reformat the test file using clang-format, it will break all these tests.
> 
> A user can always run clang-format on the output of clang-tidy if they want LLVM formatting on the result.
I agree that the tests that verify whitespace handling can have any formatting that suites their needs.

But do you need, for example, the lines in this test be indented by 4 spaces and opening braces be placed on the next line after function header? I think, we should follow the LLVM style in tests (except for the few places where other formatting is required for the purposes of the test).

http://reviews.llvm.org/D7648

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/