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

[Samuel Benzaquen]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:77
@@ +76,3 @@
+            hasRHS(expr().bind(ExpressionId)),
+            unless(hasRHS(hasDescendant(boolLiteral())))
+        ), this);
----------------
LegalizeAdulthood wrote:
> 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.
There are a few ways to do this.
A "simple" one is to not restrict the matchers and keep a list of fixed SourceRanges in the check.
Then check the list to see if you are about to fix a subrange of something you already fixed.
(if you do this, you must reset this local state at the end of the translation unit).
The good this about this approach is that the matchers are simple and it is easier to not have false negatives.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:97
@@ +96,3 @@
+
+void SimplifyBooleanExpr::matchBoolCompOpExpr(
+        ast_matchers::MatchFinder *Finder,
----------------
LegalizeAdulthood wrote:
> 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 :)
I noticed the difference in the name, but the body is pretty much the same thing.
If you think the name is good documentation, then it is fine to keep them separate.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:221
@@ +220,3 @@
+        const ast_matchers::MatchFinder::MatchResult &Result,
+        CXXBoolLiteralExpr const *BoolLiteral)
+{
----------------
LegalizeAdulthood wrote:
> 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.
>From what I can see, they (most?) all:
 - Replace the full expression with a subexpression.
 - Optionally add a prefix (a '!') to the replacement.
For all the cases that do this, you can handle them with the same check() logic. Each case would need to:
 - bind() the full expression that needs to be replaced.
 - bind() the subexpression that will be the replacement.
 - optionally mention whether it needs the "!" prefix. You can do this by binding any arbitrary node.

Then you can have code like:


```
void SimplifyBooleanExpr::matchBoolBinOpExpr(MatchFinder *Finder, bool Value,
                                             const char *const OperatorName) {
  Finder->addMatcher(
      binaryOperator(expr().bind("Full"),
                     isExpansionInMainFile(), hasOperatorName(OperatorName),
                     hasLHS(boolLiteral(equals(Value)),
                     hasRHS(expr().bind("Replacement")),
                     unless(hasRHS(hasDescendant(boolLiteral())))),
      this);

void SimplifyBooleanExpr::matchExprCompOpBool(MatchFinder *Finder, bool Value,
                                              const char *const OperatorName) {
  Finder->addMatcher(
      binaryOperator(
          expr().bind("Full"),
          expr().bind("Negate"),
          isExpansionInMainFile(), hasOperatorName(OperatorName),
          hasLHS(expr().bind(ExpressionId)).bind("Replacement"),
          unless(hasLHS(boolLiteral())),
          unless(hasLHS(hasDescendant(boolLiteral()))),
          hasRHS(hasDescendant(boolLiteral(equals(Value))))),
      this);
}


void SimplifyBooleanExpr::check(const ast_matchers::MatchFinder::MatchResult &Result)
{
  // Handle all the simple cases (are they all like this?)
  if (const Expr* Replacement = Result.Nodes.getNodeAs<Expr>("Replacement")) {
    const Expr* Full = Result.Nodes.getNodeAs<Expr>("Full");
    StringRef Prefix = Result.Nodes.getNodeAs<Expr>("Negate") ? "!" : "";
    diag(Replacement->getLocStart(), SimplifyDiagnostic)
        << FixItHint::CreateReplacement(Full->getSourceRange(),
                                        (Prefix + getText(Result, *Replacement)).str());
  } else {
    // Handle other cases that do not have simple replacements.
  }
}

```

http://reviews.llvm.org/D7648

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