[PATCH] Add readability-simplify-boolean-expr check to clang-tidy
Richard
legalize at xmission.com
Sat Mar 7 11:04:46 PST 2015
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:20
@@ +19,3 @@
+
+StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+ return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
alexfh wrote:
> alexfh wrote:
> > LegalizeAdulthood wrote:
> > > alexfh wrote:
> > > > Is this function used only once? Maybe inline it then?
> > > I'm getting conflicting reviews here.
> > >
> > > One reviewer wants me not to write inline on small functions and you're asking me to write `inline`.
> > >
> > > I honestly don't care which, but please tell me which request I should follow :-).
> > We're talking about different ways to inline a function. I mean inlining on the source code level: take the function body and use it in the single place that invokes it.
> >
> > Sorry for the ambiguity ;)
> What about this?
OK, so the first `getText` overload is on a source range.
The next `getText` is an overload on the template argument, so you get one instantiation per template argument type. They all call the first overload after extracting the source range for the node type.
Effectively, the first overload is the non-template parameter varying part of the algorithm and the second overload is the template parameter varying part.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:32
@@ +31,3 @@
+const auto LeftBooleanLiteralId = "bool-op-expr-yields-bool";
+const auto RightExpressionId = "bool-op-expr-yields-expr";
+const auto RightBooleanLiteralId = "expr-op-bool-yields-bool";
----------------
alexfh wrote:
> This form is rather exotic in Clang and LLVM code. I'd still prefer `const char X[] = "...";`.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+ const char *const Id) {
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Here and in a few other places in this patch: please use `const X *` instead of `X const *` (and the same for references) for consistency with other clang-tidy code.
> Darnit, I thought I got all those :-). My fingers put the `const` on the right (how else do you write const pointer to const character?).
Fixed.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:54
@@ +53,3 @@
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+ const char *const Id) {
+ CXXBoolLiteralExpr const *Literal =
----------------
alexfh wrote:
> alexfh wrote:
> > LegalizeAdulthood wrote:
> > > alexfh wrote:
> > > > Please use `StringRef` instead of `const char*` wherever it makes sense.
> > > I'm happy to do that with some guidelines on what "makes sense". In this case it is character literals coming down from the place where you were saying you preferred `const char Id[] = ...`.
> > >
> > > What am I gaining by using `StringRef` here?
> > "Makes sense" here means roughly "everywhere where you pass a string, and not a raw pointer to const char".
> >
> > > What am I gaining by using StringRef here?
> >
> > Peace of mind (in some sense): when you see a StringRef argument, you know that there is no expensive copying or calculation of the length going on (except where you create a StringRef from a char pointer) . Just use StringRef everywhere where you need a constant string (or a fragment of a string), and there's a buffer with sufficient lifetime (string literal, std::string) somewhere.
> >
> > You also get fewer code review comments to deal with ;)
> ditto
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:57
@@ +56,3 @@
+ Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
+ return Literal &&
+ Result.SourceManager->isMacroBodyExpansion(
----------------
alexfh wrote:
> alexfh wrote:
> > LegalizeAdulthood wrote:
> > > alexfh wrote:
> > > > nit: It looks better with parentheses and makes operator precedence clear:
> > > >
> > > > return (Literal &&
> > > > Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
> > > > ? nullptr
> > > > : Literal;
> > > >
> > > Talk to clang-format. I'm not using any of my own whitespace preferences, just ramming it through clang-format and taking what it gives me.
> > clang-format doesn't add or remove parentheses. But if you add parentheses here yourself, you'll get the formatting as in the comment above.
> >
> > So I ask you to add parentheses and re-run clang-format on this line.
> >
> > Sorry again for not being clear.
> ditto
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:66
@@ +65,3 @@
+SimpleReturnsBool(bool Value, char const *const Id = nullptr) {
+ return returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
+}
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > How about putting this into the `ReturnsBool` function?
> > >
> > > auto SimpleReturnsBool =
> > > returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
> > I went through a couple iterations on this where I had some auto lambdas and then went to named functions with explicit return types. For this one I don't think the auto is less readable.
> Note, there's no lambda needed, and no function as well. You just need a certain matcher to use in a more complex matcher. `auto SomeMatcher = someMatcher(...);` is just the simplest way to write it. The function would make sense if you called it with different arguments, e.g.:
>
> return anyOf(
> SimpleReturnsBool(Value, SomeId),
> compoundStmt(statementCountIs(1), has(SimpleReturnsBool(Value, SomeOtherId))));
>
> Otherwise, it's just simpler and more readable without a separate function:
>
> auto SimpleReturnsBool =
> returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
> return anyOf(SimpleReturnsBool,
> compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:236
@@ +235,3 @@
+
+void SimplifyBooleanExpr::replaceWithLeftBooleanLiteral(
+ const MatchFinder::MatchResult &Result,
----------------
sbenza wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > Any luck replacing `replaceWith*` methods with a single one as Samuel suggested?
> > I haven't done the experimenting yet, but I'm not hopeful. They really are all just different enough that I haven't yet seen a way to unify them.
> I'm sorry, but I have not seen one example that is really different.
> They all do the same:
>
> ```
> diag(<SomeLocationInTheExpression>, SimplifyOperatorDiagnostic)
> << FixItHint::CreateReplacement(
> <TheRangeOfTheFullExpression>,
> <OptionalPrefix> + getText(Result, <TheSubExpressionWeSelectedAsReplacement>));
> ```
> I posted code on how to do this generically for all (most?) the cases. It does not hard code binary operations or anything else. Only 3 things are needed here and the matcher can bind to generic ids.
>
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:300
@@ +299,3 @@
+ const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
+ const auto Terminator =
+ (If->getElse()->getStmtClass() == Stmt::StmtClass::CompoundStmtClass)
----------------
alexfh wrote:
> We use `auto` when it's clear what's behind it or the alternative isn't better. The most frequent use of auto is when the type (or its important part) is spelled in the RHS of the initialization. It also frequently makes sense to use `auto` in range-based for loops.
>
> Here I would clearly use `StringRef` instead.
>
> The comment applies to other usages of auto in this file as well.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:301
@@ +300,3 @@
+ const auto Terminator =
+ (If->getElse()->getStmtClass() == Stmt::StmtClass::CompoundStmtClass)
+ ? ";"
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > isa<CompoundStmt>(If->getElse()) ? ";" : "";
> Nice! I will be using that.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:304
@@ +303,3 @@
+ : "";
+ const auto Replacement = "return " + StringRef(Negated ? "!" : "") + "(" +
+ getText(Result, *If->getCond()) + ")" + Terminator;
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > And here the use of auto is simply dangerous: this creates a Twine object that is only supposed to be used as a temporary (because if your `getText()` returned `std::string`, the Twine object would point to a deleted memory region right after the end of the expression). Please use `std::string` instead.
> > My head is spinning with all this farbling around between `std::string`, `StringRef` and `Twine`. Is the proper usage of these string classes documented somewhere so that I can refer to it while preparing a patch and spend less time churning the patch just to make these adjustments?
> Some documentation is here:
> http://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes
>
> You can also read class comments on StringRef and Twine.
>
> What's important here, is that any `StringRef + something` that compiles yields a Twine. And one dangerous case is `StringRef + temporary std::string`. This creates a Twine pointing to a temporary `std::string`. The temporary string gets deleted right at the end of the expression, and the Twine continues to point to the deleted memory. So one should (almost) never store a Twine. The correct way to use it is:
>
> std::string Result = (llvm::Twine("...") + "..." + someFunctionReturningAStdStringOrAStringRef() + ...).str();
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:306
@@ +305,3 @@
+ getText(Result, *If->getCond()) + ")" + Terminator;
+ const auto Start =
+ Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
----------------
alexfh wrote:
> alexfh wrote:
> > Also not the best use for auto. Please use `SourceLocation` instead.
> ditto
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:62
@@ +61,3 @@
+
+internal::VariadicOperatorMatcher<internal::BindableMatcher<Stmt> &,
+ internal::BindableMatcher<Stmt>>
----------------
sbenza wrote:
> This type is an implementation detail.
> Why not simply internal::Matcher<Stmt> ?
Fixed.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:21
@@ +20,3 @@
+
+inline std::string getText(
+ const ast_matchers::MatchFinder::MatchResult &Result,
----------------
sbenza wrote:
> Don't force a conversion to std::string. Keep this as StringRef to avoid the cost of the string.
Fixed.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:34
@@ +33,3 @@
+
+char const *const LeftBooleanLiteralId = "bool-op-expr-yields-bool";
+char const *const RightExpressionId = "bool-op-expr-yields-expr";
----------------
alexfh wrote:
> I'd go with
> const char Something[] = "...";
> as it's more readable due to fewer `const`s.
Fixed.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:51
@@ +50,3 @@
+
+inline CXXBoolLiteralExpr const *getBoolLiteral(
+ MatchFinder::MatchResult const &Result,
----------------
LegalizeAdulthood wrote:
> 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.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+ MatchFinder::MatchResult const &Result,
+ const char *const Id)
+{
----------------
LegalizeAdulthood wrote:
> 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.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:77
@@ +76,3 @@
+ hasRHS(expr().bind(ExpressionId)),
+ unless(hasRHS(hasDescendant(boolLiteral())))
+ ), this);
----------------
sbenza wrote:
> 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.
I'm investigating alternatives to the current implementation.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:210
@@ +209,3 @@
+ Expr const *BoolLiteral,
+ std::string const Prefix)
+{
----------------
sbenza wrote:
> StringRef
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:221
@@ +220,3 @@
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ CXXBoolLiteralExpr const *BoolLiteral)
+{
----------------
LegalizeAdulthood wrote:
> 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.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:282
@@ +281,3 @@
+ diag(Ternary->getTrueExpr()->getLocStart(),
+ "Redundant boolean literal in ternary expression result.")
+ << FixItHint::CreateReplacement(
----------------
alexfh wrote:
> Please use the style of Clang warnings: first letter is lower-case, no trailing period.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.h:23-34
@@ +22,14 @@
+/// Examples:
+/// if (b == true) becomes if (b)
+/// if (b == false) becomes if (!b)
+/// if (b && true) becomes if (b)
+/// if (b && false) becomes if (false)
+/// if (b || true) becomes if (true)
+/// if (b || false) becomes if (b)
+/// e ? true : false becomes e
+/// e ? false : true becomes !e
+/// if (true) t(); else f(); becomes t();
+/// if (false) t(); else f(); becomes f();
+/// if (e) return true; else return false; becomes return (e);
+/// if (e) return false; else return true; becomes return !(e);
+///
----------------
alexfh wrote:
> Some beutification wouldn't harm, e.g.:
> /// `if (b == true)` becomes `if (b)`
> /// `if (b == false)` becomes `if (!b)`
> /// `if (b && true)` becomes `if (b)`
> /// `if (b && false)` becomes `if (false)`
> /// `if (b || true)` becomes `if (true)`
> /// `if (b || false)` becomes `if (b)`
> /// `e ? true : false` becomes `e`
> /// `e ? false : true` becomes `!e`
> /// `if (true) t(); else f();` becomes `t();`
> /// `if (false) t(); else f();` becomes `f();`
> /// `if (e) return true; else return false;` becomes `return (e);`
> /// `if (e) return false; else return true;` becomes `return !(e);`
>
Fixed.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.h:19
@@ +18,3 @@
+
+/// \brief Replace copy and swap tricks on shrinkable containers with the
+/// \c shrink_to_fit() method call.
----------------
LegalizeAdulthood wrote:
> sbenza wrote:
> > Comment is a copy-paste from some other check.
> D'oh! Thanks for catching this and your other comments as well, they've been very helpful.
Fixed
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.h:26
@@ +25,3 @@
+class SimplifyBooleanExpr : public ClangTidyCheck {
+public:
+ SimplifyBooleanExpr(StringRef Name, ClangTidyContext *Context)
----------------
alexfh wrote:
> Please clang-format the code with the LLVM style.
Fixed.
================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:11
@@ +10,3 @@
+bool ab = true == a1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant boolean constant supplied to boolean operator. [readability-simplify-boolean-expr]
+// CHECK-FIXES: {{^}}bool ab = a1;{{$}}
----------------
alexfh wrote:
> Please leave each distinct error message completely only once, and shorten all other copies of it.
Fixed
http://reviews.llvm.org/D7648
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list