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

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+    else
+      SR.setEnd(ChildFileRange->getEnd());
+  }
----------------
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?


================
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) {
----------------
sammccall wrote:
> kadircet wrote:
> > `hasOnlyRootStmtsAsChildren` ?
> nit: I think this would be clearer as
> `canBeRootStmt(const Node*)` and write the callsite as `llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though
Replaced with isRootStmt


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc,
+                                                  const SourceManager &SM,
----------------
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?


================
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:
> 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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+  tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+  NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+              tooling::ExtractionSemicolonPolicy SemicolonPolicy,
----------------
sammccall wrote:
> this is just initializing public fields, drop the constructor?
> (The callsite is clearer here if you initialize them by name)
`ExtractionSemicolonPolicy` doesn't have a default constructor.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+  if (WithTypeAndQualifiers) {
+    if (IsConst)
+      Result += "const ";
----------------
kadircet wrote:
> why don't we infer this const from QualType ?
In a future patch we will want to use heuristics like has the variable been assigned, was it passed as a non-const reference, was its address taken, etc. 


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr<ExtractionZone> ExtZone;
+};
----------------
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?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:611
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
----------------
sammccall wrote:
> wait, what?
> expressions that *aren't statements* shouldn't be extracted.
> But what's wrong with this one?
> (Many useful statements are expressions, such as function calls)
For now we don't extract if the selection is an expression. I've added a fixme to change that to sub-expressions only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526