Page MenuHomePhabricator

[clang-tidy] Added option to readability-else-after-return
ClosedPublic

Authored by njames93 on Jun 29 2020, 5:15 PM.

Details

Summary

Added a 'RefactorConditionVariables' option to control how the check handles condition variables

Diff Detail

Event Timeline

njames93 created this revision.Jun 29 2020, 5:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:15 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
200

Please use single back-ticks for option name.

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
67

Please use single back-ticks for option values. Same below.

njames93 updated this revision to Diff 274353.Jun 30 2020, 1:02 AM

Address doc backticks.

njames93 marked 2 inline comments as done.Jun 30 2020, 1:28 AM
aaron.ballman added inline comments.Jun 30 2020, 5:40 AM
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
67

Emit -> emit

74

The -> the

I'm a bit unclear on what "attempt to refactor" means -- I sort of understand it to mean that if this option is true then the check will not produce a fix-it for variables defined inside the condition of the if statement that is used in the else branch, but will produce a diagnostic. However, the check behavior seems to also remove the diagnostic in this case (not just the fix-it), so I'm not certain I'm reading this right.

njames93 marked an inline comment as done.Jun 30 2020, 6:17 AM
njames93 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
74

Good spot, the check behaviour is also removing the diagnostic as well as the fix it.
That behaviour should probably be changed to removing the Fix-It when this option is false, but then diagnostic behaviour should follow what WarnOnUnfixable dictates.

aaron.ballman added inline comments.Jun 30 2020, 6:59 AM
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
74

I think that makes sense. Then the option can be renamed to remove the "Refactor" bit and probably be something more like WarnOnConditionVariables or something?

njames93 updated this revision to Diff 274473.Jun 30 2020, 7:36 AM

Renamed new option to WarnOnConditionVariables

aaron.ballman added inline comments.Jun 30 2020, 8:16 AM
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
193

Would it make sense to hoist this into the previous if statement so we don't bother checking the condition var use in the first place if we're just going to ignore the results?

228

Same here.

njames93 marked an inline comment as done.Jun 30 2020, 9:14 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
193

That wouldn't work, we need to see if there is a condition variable that needs refactoring first before we can disregard it, Or am I missing something?

aaron.ballman added inline comments.Jun 30 2020, 9:18 AM
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
193

I was suggesting:

if (WarnOnConditionVariables && checkConditionVarUsageInElse(If)) {
 ...
}
if (WarnOnConditionVariables && checkInitDeclUsageInElse(If) {
...
}

The effect is that we don't bother checking for the situations we weren't going to warn about anyway. But maybe I'm the one missing something?

njames93 marked an inline comment as done.Jun 30 2020, 10:05 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
193

I think you may be this time. The reason being those if statements have a return at the end. If I followed how you have it layed out, those returns would never hit regardless of whether there was a condition variable that needed handling or not.

aaron.ballman accepted this revision.Jun 30 2020, 10:24 AM

LGTM once the documentation nit is fixed up.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
193

Oh! So you're talking about the case where IsLastInScope and WarnOnUnfixable are both false, okay I see that now. I think the logic could still be rearranged to avoid doing work you're just going to immediately throw away, but I don't think it's critical (or really needed for this patch).

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
74

Still have to fix the capitalization issues with The and Emit.

This revision is now accepted and ready to land.Jun 30 2020, 10:24 AM
This revision was automatically updated to reflect the committed changes.