kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:68
+ bool hasOnlyRootStmtChildren();
+ // We only support extraction of RootStmts. A RootStmt as a statement that is
+ // fully selected including all of it's children.
----------------
Could you move the definition of `RootStmt` to class definition? Since it is
used in a lot more places than just this one.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:68
+ bool hasOnlyRootStmtChildren();
+ // We only support extraction of RootStmts. A RootStmt as a statement that is
+ // fully selected including all of it's children.
----------------
kadircet wrote:
> Could you move the definition of `RootStmt` to class definition? Since it is
> used in a lot more places than just this one.
> We only support extraction of RootStmts.
Can you also mention the reasoning/implications ?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:71
+ // Returns the (unselected) parent of all RootStmts.
+ static const Node *getParentOfRootStmts(const Node *CommonAnc);
+ // Find the union of source ranges of all child nodes of Parent. Returns an
----------------
Can you make this and the `computeEnclosingFunction{,Rng}` free functions
instead of static methods?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:74
+ // inavlid SourceRange if it fails to do so.
+ SourceRange computeSourceRng();
+ // Finds the function in which the source lies.
----------------
haven't checked the implementation yet, but naming and the comments seem to be
ambiguous.
Do you mean *selected* childrens of `Parent`, instead of *all*? Because IIUC,
`Source` refers to the part of the function body that is selected, not all of
it, right?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:222
+
+public:
+ std::string FuncName = "extracted";
----------------
comments for some of the fields like `SemicolonPolicy` would be nice.
also most of these looks like implementation details, consider moving internal
ones to private
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:286
+
+// Captures information about the source.
+class CapturedSourceInfo {
----------------
could you give more details on what is collected and how?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:360
+
+CapturedSourceInfo::DeclInformation &
+CapturedSourceInfo::getDeclInformationFor(const Decl *D) {
----------------
why not move this and 4 following functions into astvisitor?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65526/new/
https://reviews.llvm.org/D65526
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
