Page MenuHomePhabricator

[clangd] Reduce availability of extract function
AcceptedPublic

Authored by kadircet on Aug 5 2020, 1:22 PM.

Details

Reviewers
sammccall
Summary

This patch introduces hoisting detection logic into prepare state with
a partial AST traversal of the enclosing function.

We had some complaints from the users about this code action being almost always
available but failing most of the time. Hopefully this should reduce that noise.

The latency/correctness tradeoff is a bunch of hand-waving, but at least today
we don't have any other actions that are available on selection of statements,
so when we get to do the traversal it is quite likely that all the other checks
will bail out early. But this is still up for discussion, I am happy to abandon
the patch if you believe this is not practical.

Diff Detail

Event Timeline

kadircet created this revision.Aug 5 2020, 1:22 PM
kadircet requested review of this revision.Aug 5 2020, 1:22 PM
sammccall accepted this revision.Aug 21 2020, 1:47 AM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
281

Maybe obvious but "if any are, we can't perform extraction, as we don't support hoisting".

291

I think this would be clearer as two for loops:

  • one filling RootStmts. i.e. the old generateRootStmts, though I do like inlining it here as it's initiaziling the ExtractionZone which really is this function's job
  • the other looping over it as part of the big block of analysis that the comment applies to

As far as I can tell, the analysis doesn't have any side-effects, so I'd move it to a separate function (either a free function that you'd call here, or a member so the tweak can if (MaybeExtZone && MaybeExtZone->canApply()) or so. Again this is because mixing initialization and complex validation can make the data flow non-obvious.

Alternatively, maybe we can unify/reuse this logic? Surely it has a lot in common with captureZoneInfo() - is it really cheaper to run?

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