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