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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits