Page MenuHomePhabricator

[clang-tidy] Handle member variables in readability-simplify-boolean-expr
ClosedPublic

Authored by LegalizeAdulthood on Jan 4 2019, 10:41 AM.

Diff Detail

Event Timeline

LegalizeAdulthood created this revision.

Fix typo in id

No, really, fix typo in id

lebedev.ri retitled this revision from Handle member variables in readability-simplify-boolean-expr to [clang-tidy] Handle member variables in readability-simplify-boolean-expr.Jan 4 2019, 10:47 AM
lebedev.ri removed a reviewer: cfe-commits.
lebedev.ri set the repository for this revision to rCTE Clang Tools Extra.
lebedev.ri added a project: Restricted Project.
lebedev.ri added a subscriber: cfe-commits.

Thanks for the patch. Is this revision dependent on D56303 (or other way around)?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
476

The const for these locals in uncommon in clang-tidy, given its a value type. I am not strongly against them, but would prefer consistency.

test/clang-tidy/readability-simplify-bool-expr-members.cpp
5

I didn't see a test that utilized this struct, did I overlook it?

Implicit conversion to bool-cases would be interesting as well. Maybe implicit conversion to integral in general.

36

Would bitfields with a single bit be of interest as well?

161

For this case the logical inversion is simple an obviously <=, but what if the condition is more complex?

I would personally prefer !(i < 20) instead + complex logical expressions as additional test. They would be interesting for the if cases, too.

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

LegalizeAdulthood marked 4 inline comments as done.Jan 5 2019, 9:33 AM
LegalizeAdulthood added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
476

I can undo the const.

test/clang-tidy/readability-simplify-bool-expr-members.cpp
5

Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove it.

Implicit conversion cases are covered by the other file. Here, I'm just testing the cases that interact with member variables because the matcher needs to use memberExpr() and not just declRefExpr()

36

That could be an enhancement, but it's not addressed by this patch.

161

If you have complex conditions, you can run the check repeatedly until you reach a fixed point.

Using !(x) instead of the reverse operator could be added as an enhancement, but it is not covered by this patch.

JonasToth added inline comments.Jan 5 2019, 10:26 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
476

Ok, that would be great.

test/clang-tidy/readability-simplify-bool-expr-members.cpp
5

Which other file? The other revision?

161

But the current version is the enhancement.
The complex condition, like i < 20 && i > 0 will be the common case, so the inversion (with parens) should always be used (!(i < 20 && i > 0)) and specializations can be done later.

LegalizeAdulthood marked 6 inline comments as done.Jan 5 2019, 1:57 PM
LegalizeAdulthood marked 5 inline comments as done.Jan 5 2019, 2:02 PM
LegalizeAdulthood added inline comments.
test/clang-tidy/readability-simplify-bool-expr-members.cpp
5

I based this new file off readability-simplify-bool-expr.cpp that is already there. It has all the main test cases for this check.

161

No, the current version is a bug fix. The simplification of boolean expressions was already there, but it didn't handle member variables properly.

You're asking for an additional enhancement about the way boolean expressions are simplified. That's fine for an additional enhancement of the check, but it is not the point of this patch.

The complex expression you're discussing in your comment is not changed by this change, nor is it changed by the existing check.

LegalizeAdulthood marked 2 inline comments as done.

Update from review comments

JonasToth added inline comments.Jan 5 2019, 2:07 PM
test/clang-tidy/readability-simplify-bool-expr-members.cpp
161

I see, then I did misunderstand these changes.

JonasToth accepted this revision.Jan 5 2019, 2:17 PM

Did you check if the changes actually fix the problematic behaviour? Did you run it over any project and did the code-transformation break the code, false positives, ...?

Otherwise LGTM

This revision is now accepted and ready to land.Jan 5 2019, 2:17 PM
LegalizeAdulthood marked an inline comment as done.Jan 5 2019, 2:29 PM

Did you check if the changes actually fix the problematic behaviour? Did you run it over any project and did the code-transformation break the code, false positives, ...?

Otherwise LGTM

The test cases that I added covered the behavior described in the bug report. I didn't run it over clang to see if it introduced any false positives, I can do that. However, when I originally wrote this check it was pretty well tested and I have confidence that I haven't regressed. However, it's still worth testing, so I will do that.

Uh... I'm developing on Windows, which doesn't have a solution for generating compile_commands.json, so I can't easily test this on the entire clang codebase. I will test on a source file based on the original bug report.

I managed to do some limited testing on the original source file from the bug report in lldb and verified that the correct fix was applied there. I also tried a few other files and verified no false positives.

I managed to do some limited testing on the original source file from the bug report in lldb and verified that the correct fix was applied there. I also tried a few other files and verified no false positives.

Thanks. That is no issue, I will run this check locally over multiple projects once committed. If I find issues I will report back!

@LegalizeAdulthood your stuck on the commit right things right? If you want I can commit for you (maybe even later) as you said you are limited on time.

@LegalizeAdulthood your stuck on the commit right things right? If you want I can commit for you (maybe even later) as you said you are limited on time.

Yes, please. I haven't figured out what the new commit procedure is or how I can submit. If you could submit this for me, that would help me out, thanks.

Thank you for the patch and sorry for the long delay.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 5:33 AM