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

[Richard]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:97
@@ +96,3 @@
+
+void SimplifyBooleanExpr::matchBoolCompOpExpr(
+        ast_matchers::MatchFinder *Finder,
----------------
sbenza wrote:
> 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.
While they may be "pretty much the same", the differences are important because the replacements are very different due to short-circuiting with `&&` and `||`.

If there were no short-circuiting then it would be the simpler thing you're suggesting.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:221
@@ +220,3 @@
+        const ast_matchers::MatchFinder::MatchResult &Result,
+        CXXBoolLiteralExpr const *BoolLiteral)
+{
----------------
sbenza wrote:
> 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.
>   }
> }
> 
> ```
I'll take a look at this and see what I can do.

http://reviews.llvm.org/D7648

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