Page MenuHomePhabricator

[clang-tidy] enhance modernize-use-bool-literals to check ternary operator
ClosedPublic

Authored by omtcyfz on Aug 6 2016, 4:31 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyfz updated this revision to Diff 67089.Aug 6 2016, 4:31 PM
omtcyfz retitled this revision from to [clang-tidy] enhance modernize-use-bool-literals to check ternary operator.
omtcyfz updated this object.
omtcyfz added reviewers: alexfh, Eugene.Zelenko.
omtcyfz added a subscriber: cfe-commits.
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added subscribers: Eugene.Zelenko, Prazek.
Prazek added inline comments.Aug 6 2016, 6:44 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
38–43 ↗(On Diff #67089)

Do I understand it correctly, that you have to have this 2 anyOf matchers, because if you would have it in one then because anyOf is lazy, it would not bind to the second branch?

I think it would be good to write some comment about it, because on the first sight it looks like it could be simplified.

test/clang-tidy/modernize-use-bool-literals.cpp
119 ↗(On Diff #67089)

Please add a test with only one branch with constant, and none of them.
Also one test like
bool Result = Value == 1 ? true : 0;
or
bool Result = Value == 1 ? false : bool(0);

omtcyfz updated this revision to Diff 67095.Aug 7 2016, 1:59 AM
omtcyfz removed rL LLVM as the repository for this revision.

Comment anyOf() part in check's matcher and extend testset.

omtcyfz marked 2 inline comments as done.Aug 7 2016, 2:00 AM
omtcyfz added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
38–43 ↗(On Diff #67089)

Do I understand it correctly, that you have to have this 2 anyOf matchers, because if you would have it in one then because anyOf is lazy, it would not bind to the second branch?

Yes, exactly. In order to bind both I do anyOf().

I think it would be good to write some comment about it, because on the first sight it looks like it could be simplified.

Done.

test/clang-tidy/modernize-use-bool-literals.cpp
119 ↗(On Diff #67089)

Added a bunch of new tests.

Prazek accepted this revision.Aug 7 2016, 8:41 AM
Prazek added a reviewer: Prazek.

LGTM, but wait a day, maybe someone will have other comments.

This revision is now accepted and ready to land.Aug 7 2016, 8:41 AM
Prazek edited edge metadata.
Prazek added a project: Restricted Project.
omtcyfz marked 2 inline comments as done.Aug 7 2016, 8:42 AM

Thanks.

Yes, sure.

aaron.ballman edited edge metadata.Aug 7 2016, 12:08 PM

Would it also make sense to support BinaryConditionalOperator as well as the ternary operator?

clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

Any reason not to name the bind "literal" in all three cases? That eliminates the need for the loop entirely, since check() will trigger for each instance of a match.

omtcyfz added inline comments.Aug 7 2016, 12:16 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

It doesn't make sense to try binding both TrueExpression and FalseExpression literals to a single value.

aaron.ballman added inline comments.Aug 7 2016, 12:19 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

Why? In all three cases, you don't care what matched, just that *some* case is matched. None of the logic in check() relies on which part of the expression is matched.

omtcyfz added inline comments.Aug 7 2016, 12:27 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

Well, in case of second matcher I may have two literals matched upon triggering. I don't understand how I could possibly get two literals bound to one value after one matcher got triggered.

Am I missing something?

aaron.ballman added inline comments.Aug 7 2016, 12:30 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

One matcher isn't what's getting triggered then, is it? I could be wrong on this point, but I thought that in that case, check() would be called twice, once for each literal. Is that not the case?

omtcyfz added inline comments.Aug 7 2016, 12:36 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

From what I understand about how matchers work, it is not. Plus, I checked (just named everything "literal" and removed for (const auto ... just to double check in case I was wrong.

aaron.ballman added inline comments.Aug 7 2016, 12:37 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

Thank you for checking -- I learned something new! :-)

omtcyfz added inline comments.Aug 7 2016, 12:40 PM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

No problem :)

One case in which your solution would work is splitting second matcher into two: one with hasTrueExpression and one with hasFalseExpression, then binding to "literal" would work. But it'll be inefficient and three names of bindings don't seem like too much hardcoding after all.

aaron.ballman accepted this revision.Aug 7 2016, 12:48 PM
aaron.ballman edited edge metadata.

LGTM! We may want to consider BinaryConditionalOperator as well, but that can be in as a separate patch.

clang-tidy/modernize/UseBoolLiteralsCheck.cpp
51–52 ↗(On Diff #67095)

I agree, that would be a step backwards.

omtcyfz updated this revision to Diff 67102.Aug 7 2016, 12:52 PM
omtcyfz edited edge metadata.

Added ternary operator example to docs.

LGTM! We may want to consider BinaryConditionalOperator as well, but that can be in as a separate patch.

Well, I personally have never seen BinaryConditionalOperator in my life.

And honestly I'm not a fan of supporting GNU extensions - what makes sense to me is to use BinaryConditionalOperator in a check which suggests not using GNU extensions.

Anyway, I'd be glad if @alexfh could jump in and take a look.

Prazek added a comment.Aug 7 2016, 3:33 PM

Yea, I also have never heard of it. I don't think it is worth even discussing

alexfh requested changes to this revision.Aug 8 2016, 7:23 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
41 ↗(On Diff #67102)

I think, Aaron was talking about eachOf matcher that will actually generate a separate match for each of its submatchers.

This revision now requires changes to proceed.Aug 8 2016, 7:23 AM
aaron.ballman added inline comments.Aug 8 2016, 7:35 AM
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
41 ↗(On Diff #67102)

I *knew* we had something that would do this! Thank you for the reminder, Alex. :-)

omtcyfz updated this revision to Diff 67169.Aug 8 2016, 7:55 AM
omtcyfz edited edge metadata.
omtcyfz marked an inline comment as done.

Use shiny eachOf and bind everything to "literal" value.

clang-tidy/modernize/UseBoolLiteralsCheck.cpp
41 ↗(On Diff #67102)

Yup. Seems like I learned about something, too :)

alexfh requested changes to this revision.Aug 8 2016, 8:52 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseBoolLiteralsCheck.cpp
39 ↗(On Diff #67169)

Remove anyOf and anything. And the comment.

This revision now requires changes to proceed.Aug 8 2016, 8:52 AM
omtcyfz updated this revision to Diff 67189.Aug 8 2016, 9:49 AM
omtcyfz edited edge metadata.

Remove redundant anyOf and anything.

omtcyfz marked an inline comment as done.Aug 8 2016, 9:50 AM
alexfh accepted this revision.Aug 8 2016, 9:55 AM
alexfh edited edge metadata.

LG with a nit.

test/clang-tidy/modernize-use-bool-literals.cpp
124 ↗(On Diff #67189)

The check lines should be a bit stricter. Please change to :[[@LINE-1]]:30: warning: converting integer literal to bool. Other new patterns in this file as well.

This revision is now accepted and ready to land.Aug 8 2016, 9:55 AM
omtcyfz updated this revision to Diff 67195.Aug 8 2016, 10:08 AM
omtcyfz edited edge metadata.

More strict test messages.

omtcyfz marked an inline comment as done.Aug 8 2016, 10:09 AM
This revision was automatically updated to reflect the committed changes.