================
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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
