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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc,
+                                                  const SourceManager &SM,
----------------
SureYeaah wrote:
> SureYeaah wrote:
> > sammccall wrote:
> > > kadircet wrote:
> > > > why is this function returning a shared_ptr ?
> > > avoid shared_ptr unless there's a strong reason. `Optional<ExtractionZone>` seems fine here?
> > And store a unique_ptr to the optional in ExtractFunction?
> Because ExtractFunction needs to store a pointer/reference to ExtractionZone somehow in prepare and access it in apply.
Let's figure it out in the `ExtractFunction` this can still return an `Optional<ExtractionZone>` which you can convert into a pointer/reference later on if need be?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr<ExtractionZone> ExtZone;
+};
----------------
SureYeaah wrote:
> kadircet wrote:
> > why do you need a shared_ptr here?
> getExtractionZone creates an Optional<ExtractionZone> which needs to persist from prepare to apply. Is there a better way to store a reference to the ExtractionZone instance inside ExtractFunction?
Then why not just store `Optional<ExtractionZone>` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+  SourceRange EnclosingFuncRange;
+  std::set<const Stmt *> RootStmts;
+  SourceLocation getInsertionPoint() const {
----------------
lifetime of this field looks complicated can you add some comments on how/when it is initialized and maybe enforce immutability ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150
+  }
+  bool isRootStmt(const Stmt *S) const;
+  // The last root statement is important to decide where we need to insert a
----------------
it seems like you are rather using it to decide whether a statement is inside the zone or not?

Could you rather rename it to reflect that and add some comments?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:169
+      // FIXME: Support extraction from methods.
+      if (CurNode->ASTNode.get<CXXMethodDecl>())
+        return nullptr;
----------------
nit:
```
if (isa<CXXMethodDecl>(Func))
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172
+      // FIXME: Support extraction from templated functions.
+      if (CurNode->Parent->ASTNode.get<FunctionTemplateDecl>())
+        return nullptr;
----------------
nit:
```
if(isa<FunctionTemplateDecl>(Func))
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+    ZoneRelative DeclaredIn;
+    // index of the declaration or first reference
+    unsigned DeclIndex;
----------------
i think you mean `index of the first reference to this decl` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:486
+    // (this includes the case of recursive call to EnclosingFunc in Zone).
+    if (!VD || dyn_cast_or_null<FunctionDecl>(DeclInfo.TheDecl))
+      return false;
----------------
nit `isa<FunctionDecl>` instead of `dyn_cast`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526