[PATCH] D85354: [clangd] Reduce availability of extract function

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:291
+  llvm::SmallSet<const Decl *, 1> DeclsInExtZone;
+  for (const Node *Child : ExtZone.Parent->Children) {
+    auto *RootStmt = Child->ASTNode.get<Stmt>();
----------------
sammccall wrote:
> 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?
> I think this would be clearer as two for loops:

done.

> 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.

moved into a member named `requiresHoisting`.

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

In theory this should be always cheaper than `captureZoneInfo` as it traverses the whole enclosing function, whereas this one only traverses ast nodes that intersect with selection or come after it.
So if selection is near the beginning of the function body, or the function itself is small, the delta is likely negligible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85354/new/

https://reviews.llvm.org/D85354