Page MenuHomePhabricator

Remove else statements after returns
AbandonedPublic

Authored by JDevlieghere on Dec 11 2018, 1:08 PM.

Details

Reviewers
espindola
jfb
shafik
aprantl
Group Reviewers
Restricted Project
Summary

While writing a patch I noticed myself removing a few else statements after return statements. Rather than doing this ad-hoc I remembered there's a clang-tidy pass that does this.

https://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

So essentially what I did was run the following command over the LLDB code base:

run-clang-tidy.py -checks='-*,readability-else-after-return' -format -fix $PWD

I'm not sure if it's worth the churn, but I do believe lldb could benefit from some reduced indentation.

PS: Diff is without context, otherwise it exceeds the Phabricator limit of 8 megabytes.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 11 2018, 1:08 PM

I'm mostly on board with making these changes, as it's good LLVM style to do this. I highlighted a couple changes that might warrant a closer look.

source/API/SBProcess.cpp
1233

This function probably could use some manual refactoring, too.

source/Commands/CommandObjectTarget.cpp
2569

Can you manually double-check this one?

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
3818

what's going on here with the indentation?

source/Target/Process.cpp
1930

this also looks suspicious

5235

wouldn't hurt to manually check this one, too

source/Target/ThreadPlanStepRange.cpp
329

weird indentation again

source/Target/ThreadPlanStepUntil.cpp
246

again

vsk added a subscriber: vsk.Dec 11 2018, 2:54 PM

+ 1 to this. If there's a tidy plugin for misleading indention, that might address some of Adrian's concerns.

aprantl added inline comments.Dec 11 2018, 3:19 PM
source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp
994

This is inconsistent (do you need to re-run it until it reaches a fixpoint?

zturner added inline comments.
source/Commands/CommandObjectTarget.cpp
2569

For large complex cases like this, it might make sense to convert them to early returns first. That makes the size of the if blocks smaller so less chance of messing up.

2601–2607

Something is wrong with indentation here.

When one or the other side of the if/else test is trivial, then this rewrite is fine. When both the if and the else have a decent bit of code in them, however, removing the "} else {" and the concluding "}" obscures the fact that these are two coequal branches in the code. Before, regardless of whether I'm reading from the bottom of the function up or the top down, the structure is obvious. But when the else removed, particularly when reading from the bottom up, I can't tell this till I get to the } which happens to have a return just before it to know that. So that sort of change doesn't improve readability to my mind.

JDevlieghere abandoned this revision.Dec 11 2018, 6:50 PM

I'm going to abandon this for now until we have better tooling to address Jim's concerns here. I'm also not super happy with the formatting, which seems to be off in quite a few locations. The problem is that clang-tidy knows to reformat the source range it touched, but in this case the code below is affected. I hacked this up by having clang-format looking one line below the lines that were changed, but looks like that was not sufficient. Also a few case are covered by D55584 so definitely want to land that first.