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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:281
+
+  // Make sure declarations inside extraction zone are not accessed afterwards.
+  // This performs a partial AST traversal proportional to the size of the
----------------
Maybe obvious but "if any are, we can't perform extraction, as we don't support hoisting".


================
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>();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85354