[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Kadir Cetinkaya via Phabricator via cfe-commits]

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