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

[Samuel Benzaquen]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:21
@@ +20,3 @@
+
+inline std::string getText(
+        const ast_matchers::MatchFinder::MatchResult &Result,
----------------
Don't force a conversion to std::string. Keep this as StringRef to avoid the cost of the string.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:51
@@ +50,3 @@
+
+inline CXXBoolLiteralExpr const *getBoolLiteral(
+        MatchFinder::MatchResult const &Result,
----------------
There is no need to mark this inline.
Are you doing it to avoid ODR-problems? or to make the code "faster"?

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+        MatchFinder::MatchResult const &Result,
+        const char *const Id)
+{
----------------
Use StringRef instead of 'const char*'.
Here and all other 'const char*' arguments.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:77
@@ +76,3 @@
+            hasRHS(expr().bind(ExpressionId)),
+            unless(hasRHS(hasDescendant(boolLiteral())))
+        ), this);
----------------
Why does it matter if the RHS has a bool literal in it?
This would prevent something like this from matching:
  if (true && SomeFunction("arg", true))

Also, hasDescedant() and alike are somewhat expensive. Something to take into account when using them.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:97
@@ +96,3 @@
+
+void SimplifyBooleanExpr::matchBoolCompOpExpr(
+        ast_matchers::MatchFinder *Finder,
----------------
How are these different from matchBoolBinOpExpr ?

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:210
@@ +209,3 @@
+        Expr const *BoolLiteral,
+        std::string const Prefix)
+{
----------------
StringRef

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:221
@@ +220,3 @@
+        const ast_matchers::MatchFinder::MatchResult &Result,
+        CXXBoolLiteralExpr const *BoolLiteral)
+{
----------------
All these replace look the same.
The are replacing the full expression with a subexpression.
They seem like they all could be:

  void SimplifyBooleanExpr::replaceWithSubexpression(
          const ast_matchers::MatchFinder::MatchResult &Result,
          const Expr *Subexpr, StringRef Prefix = "")
  {
      const Expr* Expression = Result.Nodes.getNodeAs<Expr>(ExpressionId);
      diag(Subexpr->getLocStart(), SimplifyDiagnostic)
          << FixItHint::CreateReplacement(Expression->getSourceRange(),
                                          (Prefix + getText(Result, *Subexpr)).str());
  }

If all the replace calls look the same, then you can also consolidate the bind ids to simplify check().
It could be like:
  if (const Expr* Sub = result.Nodes.getNodeAs<Expr>("ReplaceWithSub")) {
    replaceWithSubexpression(Result, Sub);
  } else if (...
and that might actually support all the uses cases, in which case no if/else is needed. Then check() becomes:
  void SimplifyBooleanExpr::check(const ast_matchers::MatchFinder::MatchResult &Result)
  {
    const Expr* Full = Result.Nodes.getNodeAs<Expr>(ExpressionId);
    const Expr* Sub = Result.Nodes.getNodeAs<Expr>(SubexpressionId);
    StringRef Prefix = Result.Nodes.getNodeAs<Expr>(AddNotId) ? "!" : "";
    diag(Sub->getLocStart(), SimplifyDiagnostic)
        << FixItHint::CreateReplacement(Full->getSourceRange(),
                                        (Prefix + getText(Result, *Sub)).str());
  }

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.h:19
@@ +18,3 @@
+
+/// \brief Replace copy and swap tricks on shrinkable containers with the
+/// \c shrink_to_fit() method call.
----------------
Comment is a copy-paste from some other check.

http://reviews.llvm.org/D7648

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