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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
