Page MenuHomePhabricator

[clang-tidy] readability-simplify-boolean-expr detects negated literals
ClosedPublic

Authored by njames93 on Aug 18 2020, 3:53 PM.

Diff Detail

Event Timeline

njames93 created this revision.Aug 18 2020, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2020, 3:53 PM
njames93 requested review of this revision.Aug 18 2020, 3:53 PM
aaron.ballman added inline comments.Aug 20 2020, 5:11 AM
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
66

I'd like to see this handled recursively instead so that we properly handle more esoteric logic (but it still shows up from time to time) like !!foo: https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21&search=Search

This is a case where I don't think the simplification should happen -- e.g., the code is usually subtly incorrect when converted to remove the !!. It's less clear to me whether the same is true for something like !!! being converted to !, but that's not a case I'm really worried about either.

74

Oof, but handling it here may be tricky...

289

Same here.

njames93 added inline comments.Aug 20 2020, 7:02 AM
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
66

As this is only looking for boolean literals that shouldn't matter
!!true is identical to true.
I get putting !! in front of expressions that aren't boolean has an effect, but we aren't looking for that in this case.

74

A custom matcher could be made for this that could traverse a UnaryOperator

289

This could be easily handled with some recursion.

aaron.ballman added inline comments.Aug 20 2020, 7:14 AM
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
66

Oh, derp, I saw "SimplifyBooleanExprCheck" and assumed it was simplifying arbitrary boolean expressions, not just literals. I am now far less worried about supporting this case, it's up to you if you want to do the extra work. Thank you for clarifying and sorry for my think-o!

njames93 added inline comments.Aug 20 2020, 11:19 PM
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
66

Personally I don't think the extra complexity is worth it. Usages of !true, I expect are rather low but likely show up in a few places, however I doubt anyone would use !!true.

aaron.ballman accepted this revision.Aug 21 2020, 7:08 AM

LGTM, thank you for the fix!

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
66

Agreed.

This revision is now accepted and ready to land.Aug 21 2020, 7:08 AM