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