[PATCH] D65526: [Clangd] First version of ExtractFunction

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:29
+//   - Always passed by l-value reference
+// - No return
+// - Cannot move declarations before extracting
----------------
did you mean no return *type* ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:30
+// - No return
+// - Cannot move declarations before extracting
+// - Doesn't check for broken control flow
----------------
don't understand what you meant here


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:31
+// - Cannot move declarations before extracting
+// - Doesn't check for broken control flow
+//
----------------
could you elaborate on that one?

You can ignore this comment if it is gonna be explained in the design doc(and you are planning to attach it before landing this revision)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:42
+//    methods for rendering it.
+// 4. CapturedZoneInfo uses a RAV to capture information about the extraction
+//    like declarations, existing return statements, broken control flow, etc.
----------------
Tried hard to figure out what RAV is :D could you rather say `RecursiveASTVisitor(RAV)` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:122
+    return ZoneRelative::OutsideFunc;
+  if (Loc < ZoneRange.getBegin())
+    return ZoneRelative::Before;
----------------
IIRC, `<` operator in `SourceLocation`s are merely for implementing hashing and doesn't really carry `less than` semantics. Could you rather use `SM.isBeforeInSLocAddrSpace`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:124
+    return ZoneRelative::Before;
+  if (Loc < ZoneRange.getEnd())
+    return ZoneRelative::Inside;
----------------
same here


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:133
+// Finds the function in which the zone lies.
+const Node *computeEnclosingFunction(const Node *CommonAnc) {
+  // Walk up the SelectionTree until we find a function Decl
----------------
Can you rather return a `FunctionDecl` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:152
+
+// Find the union of source ranges of all child nodes of Parent. Returns an
+// invalid SourceRange if it fails to do so.
----------------
maybe rather return an llvm::Optional/Expected ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+    else
+      SR.setEnd(ChildFileRange->getEnd());
+  }
----------------
I suppose this relies on the fact that "AST contains the nodes ordered by their begin location"? Could you add an assertion for that?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:176
+                                      const LangOptions &LangOpts) {
+  return *toHalfOpenFileRange(SM, LangOpts,
+                              EnclosingFunction->ASTNode.getSourceRange());
----------------
what makes this fail-free ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:180
+
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
----------------
This explains *what* the function is doing, but lacks the *reason why*

Could you explain the reason either in here or in the call site?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+  for (const Node *Child : Parent->Children) {
----------------
`hasOnlyRootStmtsAsChildren` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:215
+    const Node *Parent = CommonAnc->Parent;
+    // If parent is a DeclStmt, even though it's unselected, we consider it a
+    // root statement and return its parent.
----------------
why ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc,
+                                                  const SourceManager &SM,
----------------
why is this function returning a shared_ptr ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+  if (WithTypeAndQualifiers) {
+    if (IsConst)
+      Result += "const ";
----------------
why don't we infer this const from QualType ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:347
+    ZoneRelative DeclaredIn;
+    // Stores the numbering of this declaration(i for the i-th Decl)
+    unsigned DeclIndex;
----------------
what does `numbering` mean ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:480
+    // FIXME: Support Decl Hoisting.
+    if (DeclInfo.DeclaredIn == ZoneRelative::Inside &&
+        DeclInfo.IsReferencedInPostZone)
----------------
nit: move this to top of the loop


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:519
+// FIXME: assign the value returned by non void extracted function.
+llvm::Optional<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
+                                                 const SourceManager &SM,
----------------
maybe return an `llvm::Expected` so that we can tell user about the reason why refactoring failed?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr<ExtractionZone> ExtZone;
+};
----------------
why do you need a shared_ptr here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526