[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:119
+  case SelectionTree::Selection::Partial:
+    // Treat Partially selected VarDecl as completely selected since
+    // SelectionTree doesn't always select VarDecls correctly.
----------------
sammccall wrote:
> D66872 has the fix if you'd rather avoid these workarounds
Will remove this once D66872 is committed.


================
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
----------------
kadircet wrote:
> 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?
if extracting 
```
for(;;)
  int x = 0;
```
the DeclStmt is inside the zone but not a rootstmt.


================
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;
----------------
kadircet wrote:
> nit:
> ```
> if(isa<FunctionTemplateDecl>(Func))
> ```
Using `Func->isTemplated()`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+    ZoneRelative DeclaredIn;
+    // index of the declaration or first reference
+    unsigned DeclIndex;
----------------
kadircet wrote:
> i think you mean `index of the first reference to this decl` ?
It can be the index of declaration or the first reference. Since the visitor Visits Decls as well, we will encounter a reference before the Decl only if the Decl is outside the enclosing function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526