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

[Richard]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:51
@@ +50,3 @@
+
+inline CXXBoolLiteralExpr const *getBoolLiteral(
+        MatchFinder::MatchResult const &Result,
----------------
sbenza wrote:
> There is no need to mark this inline.
> Are you doing it to avoid ODR-problems? or to make the code "faster"?
I suppose it's my own coding habit to mark functions containing one or two statements as inline.  There is nothing intrinsically important about this function being inline.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+        MatchFinder::MatchResult const &Result,
+        const char *const Id)
+{
----------------
sbenza wrote:
> Use StringRef instead of 'const char*'.
> Here and all other 'const char*' arguments.
Yes, this came up in the review of readability-remove-void-arg and that change will be folded in here as well.  Thanks.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:77
@@ +76,3 @@
+            hasRHS(expr().bind(ExpressionId)),
+            unless(hasRHS(hasDescendant(boolLiteral())))
+        ), this);
----------------
sbenza wrote:
> 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.
There are two cases: either the boolean literal is the LHS or it is the RHS of a binary operator.  This is the matcher that handles the LHS being the literal and the RHS being the expression.

This code specifically excludes the case where both LHS and RHS are boolean literals so that complex expressions of constants like:

```
if (false && (true || false))
```

are handled without error -- see the nested_booleans code in the test file.  If I don't narrow the matcher in this way, then the above expression gets two "fixes" written for it, one that replaces the inner expression and another that replaces the outer expression.  Unfortunately, when this happens the combined edits produce invalid code.

I am not certain if this is an error in the engine that applies the fixes or not, but that's what you currently get.  So to prevent invalid code from being generated, I disallow one of the matches.

This check could be enhanced to do more elaborate boolean expression elimination, but realistically this is just a corner case for this checker and not something that you should expect to see in real-world code.

I'll look at seeing if there is an alternative to hasDescendant() that I can use that will do the same job.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:97
@@ +96,3 @@
+
+void SimplifyBooleanExpr::matchBoolCompOpExpr(
+        ast_matchers::MatchFinder *Finder,
----------------
sbenza wrote:
> How are these different from matchBoolBinOpExpr ?
I tried to name the functions after what they were matching.  One is matching <bool> <binop> <expr> and the other is matching <expr> <binop> <bool>.  (Sorry for the crappy notation there.)

If you didn't see the difference immediately with the names of the two functions, then I need better names!  I'm open to suggestions :)

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:221
@@ +220,3 @@
+        const ast_matchers::MatchFinder::MatchResult &Result,
+        CXXBoolLiteralExpr const *BoolLiteral)
+{
----------------
sbenza wrote:
> 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());
>   }
I'll look into seeing if I can simplify it further, but the differences come down to either the fact that they have different matchers or that the replacement text is different in the different cases.

Honestly, I abhor duplication and I've pounded on this for many hours trying to make it as small as I can and that's what you're seeing in these reviews.

http://reviews.llvm.org/D7648

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