Page MenuHomePhabricator

clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers
ClosedPublic

Authored by LegalizeAdulthood on Jan 18 2016, 11:38 PM.

Details

Summary

Expand the simplify boolean expression check to handle implicit conversion of integral types to bool
and improve the handling of implicit conversion of member pointers to bool.

Implicit conversion of member pointers are replaced with explicit comparisons to nullptr.

Implicit conversions of integral types are replaced with explicit comparisons to 0.

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers.
LegalizeAdulthood updated this object.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

Can you also update for member pointers?

21–22

To me, this does not improve readability -- I now have to think much harder about operator precedence. Including parens would help significantly, IMO, so that it becomes (i & 1) != 0.

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
60

Update for member pointers.

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

To clarify: You'd like to see parens around the expression when it is a binary operator, correct?

When it is a variable, there's no need to add parentheses.

21–22

I can explicitly state that; to me "pointers" is a term that includes member pointers.

aaron.ballman added inline comments.Jan 19 2016, 9:45 AM
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

Hmm, perhaps I am being too pedantic. I'm used to thinking of member pointers as different from pointers because it's that way in standardese.

21–22

Correct; if it's just a DeclRefExpr or unary operator then parens aren't useful; but if it's a binary operator, I think parens may help clarify.

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
60

Do I really need to explicitly say member pointer as well? It seems redundant.

I didn't update it because a pointer to a member is a pointer. When used as an implicit conversion to bool, the syntax is no different for a plain pointer than for a member pointer. If the syntax were different, I could see your point.

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

Ironically, in another review we were talking about eliminating redundant parentheses globally and these parentheses added for binary operators would be considered redundant and removed.

aaron.ballman added inline comments.Jan 20 2016, 9:40 AM
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

In the other review, I was suggesting that parens in expressions involving binary operators of differing precedence should not be removed. ;-)

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
60

A pointer to a member is not a pointer according to the language standard, and that's why it may not be a bad idea to call it out explicitly as being supported. From [basic.compound]p3, "Except for pointers to static members, text referring to “pointers” does not apply to pointers to members." I do take your point about the syntax of the implicit conversion being the same, but it would be nice to be explicit about what we support. I would just change it to say "...implicit conversion of a pointer (or pointer to member) to `bool`..."

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
60

Meh. I don't mind doing that, but the standard gets hyper-anal about how everything is called because without it the standard would be much more ambiguous. I certainly don't want clang (or clang-tidy) documentation to start reading like the C++ Standard. In other words, while such specific language is necessary in the standard, I don't think anyone is going to be confused by how I originally worded it.

LegalizeAdulthood marked 2 inline comments as done.

Update from review comments.

aaron.ballman added inline comments.Jan 25 2016, 6:54 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
192

Can we add a test to ensure that we aren't suggesting nullptr for a C program? Perhaps we could simply substitute nullptr for >= C++11 and NULL for everything else?

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

Perhaps a different idea regarding parens isn't to add/remove parens in various checks for readability, but instead have two readability checks that do different things (both disabled by default?): (1) remove spurious parens where the presence of the parens has no effect on the order of evaluation of the subexpressions, and (2) add parens where there is an operator precedence difference between the operators of two subexpressions. Both of these checks are at odds with one another (which is why I don't think it makes sense to enable them by default), but both certainly seem like they would be useful.

Thoughts on the general idea (not trying to sign either of us up for work to implement it)?

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
60

I guess I view it differently; this isn't being hyper-anal, it's precisely stating what we support so that the user doesn't have to guess. However, I'm also fine leaving it out because it isn't likely to cause confusion for too long -- a simple test gives the answer if anyone is wondering.

LegalizeAdulthood marked 2 inline comments as done.Jan 29 2016, 8:59 PM
LegalizeAdulthood added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–22

I'm of two minds on this, I'm not really coming down hard on either side. It feels like there should be a readability check for removing parenthesis to consolidate the heuristics all in one place and that check can in turn be configured by parameters.

Update from review comments.
trunk clang-format

Inline Variable

Move local function to anonymous namespace

aaron.ballman added inline comments.Feb 1 2016, 8:02 AM
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

I think I can agree with that, but I also think it depends a lot on what the purpose to the check is. If it's "improve readability regarding parens" + options that control when to remove or add, that makes sense to me. Otherwise, I think segregating the checks into one that removes (plus options) and one that adds (plus options) makes sense -- even if they perhaps use the same underlying heuristic engine and are simply surfaced as separate checks.

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

This check isn't at all about the readability of parens, so it doesn't make sense for this check to be concerned with it IMO.

aaron.ballman added inline comments.Feb 1 2016, 9:11 AM
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

Agreed; trying to suss out what the appropriate design for this particular check is. I think I've talked myself into "don't touch parens here."

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

Currently in this patch, parens are added when the expression compared to nullptr or 0 is a binary operator.

I think that is the right thing to do here so we get:

bool b = (i & 1) == 0;

instead of

bool b = i & 1 == 0;
aaron.ballman added inline comments.Feb 1 2016, 1:45 PM
clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

And we've come full circle to my original suggestion. :-D Personally, I find it to be more readable with the parens than without. I'm just worried that we have this check adding parens and another check removing them. But that can be handled in the check that removes them instead of here. My statement about not touching parens was horribly unclear, and I am sorry for that. What my brain was telling my fingers to type was: Don't worry about parens here; add them if they add clarity, don't add them if they don't add clarity. We can worry about parens categorically as part of another check.

clang-tidy/readability/SimplifyBooleanExprCheck.h
21–23

Yes, I did update the patch per your earlier review comments.

I agree this is more readable and that a future check that generally removes unnecessary parens can handle it if people don't like it.

Because there is no "right" answer, I prefer to deploy the check and await feedback from users to know if adding the parens is widely considered bad. If the general consensus is that adding the parens is bad, this check can be enhanced with an option to decide when to add them. Prior to this change, it only added them when necessary to preserve semantics.

In trying this check on llvm code base I have seen one occasion where the check produced something like !!(complicated expression), but I haven't figured that one out yet. It's on my list of things to improve with this check.

Once this change is accepted, I have another improvement waiting in the wings to simplify things like !(a < b) to (a >= b), etc. There has been some discussion earlier about how negated combinations of boolean expressions should be handled. The majority of the comments so far feel that applying DeMorgan's law generally makes for better readability, but again with readability checks not everyone agrees.

For now, I believe I have addressed all of Aaron's outstanding comments.

aaron.ballman accepted this revision.Feb 2 2016, 4:49 AM
aaron.ballman edited edge metadata.

LGTM!

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
346

Sorry for not noticing this earlier, but since we have two other in-flight patches I reviewed this morning, it caught my attention: we should not be using isExpansionInMainFile, but instead testing the source location of the matches to see if they're in a macro expansion. This is usually done with something like Result.SourceManager->isMacroBodyExpansion(SomeLoc).

I realize this is existing code and not your problem, but it should be fixed in a follow-on patch (by someone) and include some macro test cases.

This revision is now accepted and ready to land.Feb 2 2016, 4:49 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
346

There is an open bug on this that I will address. And while this is existing code, I am the author of the original check :).

I also experienced a problem when running this check on some code that did something like:

#define FOO (par[1] > 4)

// ...
bool f() {
  if (!FOO) {
    return true;
  } else {
    return false;
  }
}

So it needs improvement w.r.t. macros and that is also on my to-do list.

I'm trying to do things in incremental steps and not giant changes.

I do not have commit access. If someone could commit this for me, I would appreciate it.

Patch by Richard Thomson.

Thanks.

It's been almost a week. Is there some reason this hasn't been committed yet?

aaron.ballman closed this revision.Feb 8 2016, 6:30 AM

Commit in r260096, sorry for the delay. You should speak to Chris Lattner about getting commit privileges so that you're not blocked on one of us being available to commit on your behalf again.

This is causing build bot failures:

http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

The failure is not obvious to me, and I don't have the opportunity to debug locally right now, so I have reverted in r260097.

In article <e0420edb461dccec7b7a6f4fe09f10e8@localhost.localdomain>,

Aaron Ballman <aaron.ballman@gmail.com> writes:

aaron.ballman added a comment.

This is causing build bot failures:

http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

The failure is not obvious to me, and I don't have the opportunity to debug
locally right now, so I have reverted in r260097.

make check-all passes for me. I don't see what the problem is in the logs
either.

LegalizeAdulthood edited edge metadata.

Fixes StringRef problem that crashes tests in release builds only.

If someone could review the changes and commit this corrected version for me, that would be great.

Patch by Richard Thomson.

I have commit in r260681. You should look into obtaining commit privileges: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

~Aaron