Page MenuHomePhabricator

[clang-tidy] Handle case/default statements when simplifying boolean expressions
Needs ReviewPublic

Authored by LegalizeAdulthood on Jan 3 2019, 5:32 PM.

Details

Summary

Structure this similarly to the way replaceCompoundReturnWithCondition works.

  • Add readability-simplify-boolean-expr test cases for case stmt
  • Add readability-simplify-boolean-expr test cases for default stmt
  • Add negative test cases

Fixes #26704

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from Handle case/default statements when simplifying boolean expressions to [clang-tidy] Handle case/default statements when simplifying boolean expressions.Jan 4 2019, 2:06 PM
LegalizeAdulthood edited reviewers, added: alexfh, hokein, aaron.ballman, JonasToth; removed: cfe-commits.
LegalizeAdulthood set the repository for this revision to rCTE Clang Tools Extra.
LegalizeAdulthood added a subscriber: cfe-commits.
JonasToth added inline comments.Jan 5 2019, 5:42 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
386

const on values is uncommon in clang-tidy code. Please keep that consistent.

743

double space

745

redundant empty comment line

747

I think this assertion does not hold true from the matcher.
The matcher requires only hasDescendent(ifStmt()), but that does not ensure the first stmt is the ifStmt.

e.g.

case 10: {
  loggingCall();
  if(foo) ...

Is this excluded?
}

aaron.ballman added inline comments.Jan 5 2019, 10:43 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
511

binding -> Binding

529

binding -> Binding

Also, there is no "case statement" involved here. ;-)

533–540

The check duplication here is unfortunate -- can you factor out the hasDescendant() bits into a variable that is reused, and perhaps use anyOf(caseStmt(stuff()), defaultStmt(stuff())) rather than separate functions?

718

Don't use an identifier with the same name as a type, please.

719

Add a message to the assertion (same with the other ones).

LegalizeAdulthood marked 10 inline comments as done.Jan 5 2019, 2:17 PM
LegalizeAdulthood added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
386

I can change the code, but I don't understand the push back.

"That's the way it's done elsewhere" just doesn't seem like good reasoning.

I write const on values to signify that they are computed once and only once. What is gained by removing that statement of once-and-only-once?

533–540

I'm not a fan of duplication, either.

However, I have to know if it's a CaseStmt or DefaultStmt in the replacement code and that's tied to the id, so I'm not sure how to collapse it further.

719

I'm not sure what you're asking for. I based these asserts off the existing assert in the file.

743

What exactly is the problem?

745

Meh, it's not redundant. It's there to aid readability of a big block of text by visually separating it from the associated code.

Why is this a problem?

747

Look more carefully at the AST. CaseStmt has exactly one child. That child can either be a compound statement block (which was already correctly handled by the check) or it can be a single statement. This change enhances the check to handle the single statement child of the CaseStmt and DefaultStmt.

riccibruno added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
719

Something like assert((SwitchCase != nullptr) && "Some appropriate message blablabla!");
Keep the parentheses around the operands of != even if they are not strictly needed
since otherwise some bots will complain.

LegalizeAdulthood marked an inline comment as done.

Update from review comments

LegalizeAdulthood marked 3 inline comments as done.Jan 5 2019, 3:56 PM
LegalizeAdulthood added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
719

OK, I can do that; I wasn't sure if it was being suggested that I use some custom assert macro that took the message as a parameter.

LegalizeAdulthood marked an inline comment as done.

Update from review comments

LegalizeAdulthood marked 4 inline comments as done.

No really, update from review comments :)

aaron.ballman added inline comments.Jan 6 2019, 7:41 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
386

"That's the way it's done elsewhere" just doesn't seem like good reasoning.

Keeping the code consistent with the vast majority of the remainder of the project is valid reasoning. I am echoing the request to drop the top-level const. We don't use this pattern for non-pointer/reference types and there's not a whole lot gained by using it inconsistently.

533–540

You can bind to the castStmt() and defaultStmt() matchers to see what you get back, no? e.g., anyOf(caseStmt(stuff()).bind("which"), defaultStmt(stuff()).bind("which")) and in the check, you can use isa<> on the node named "which" to see whether you matched the case or the default label.

JonasToth added inline comments.Jan 7 2019, 8:41 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
386

Plus we do have a clang-tidy check (in the pipeline) that could do that automatically if the LLVM projects decides to go that route. So we won't suffer in the future, if we add the const.

745

Its subjective, I wouldn't do it so I thought you might have overlooked it, if we prefer this its fine from my side.

alexfh added inline comments.Jan 23 2019, 4:55 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
517

Please put the more expensive matcher (hasDescendant may be *really* expensive) last. Same below.

LegalizeAdulthood marked 4 inline comments as done.Wed, Mar 20, 12:39 PM
LegalizeAdulthood added inline comments.
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
386

You haven't addressed my point, which is that const is for values that don't change. Instead, you're just repeating "we haven't done it that way" instead of refuting the utility of const.