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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

I think we should go ahead and land this. The only points that I'd really like to see fixed is `shared_ptr`, mostly because it's a strong signal there's something complicated going on and I don't think (or hope) there is!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+  return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};
----------------
sammccall wrote:
> use `printType` from AST.h?
> 
> (You'll want to drop qualifiers/refs before calling that, but it's not at all obvious from the function name here that they're dropped, so that should be at the callsite anyway)
Second half is not done: the suggestion is that it's really confusing where spellType is called that it drops qualifiers, so just I'd inline this into the callsite.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+  if (WithTypeAndQualifiers) {
+    if (IsConst)
+      Result += "const ";
----------------
SureYeaah wrote:
> 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. 
I think the point is that the QualType in parameter could/should represent the *parameter* type, not the type of the variable being captured.
e.g. it could be  `const std::string&` even if the original variable was just `std::string`.

This seems the more natural model (the struct is called Parameter, not CapturedVariable or Argument) but there may be reasons not to do this.


================
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?
shared ownership is less common and harder to reason about than exclusive ownership

Generally we prefer deciding where ownership should reside (T or Optional<T> or unique_ptr<T>), and where you should have an unowned reference (T& or T*).

In this case, findExtractionZone() should ideally return `Optional<ExtractionZone>`, after prepare() the ExtractFunction class should own it (as a Optional<ExtractionZone>), and apply should pass it around as a const ExtractionZone&.

If it turns out ExtractionZone isn't a movable type, then `Optional<ExtractionZone>` won't work and you'll need `unique_ptr<ExtractionZone>` instead. But shared_ptr shouldn't be needed regardless.


================
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.
----------------
D66872 has the fix if you'd rather avoid these workarounds


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+  SourceRange EnclosingFuncRange;
+  std::set<const Stmt *> RootStmts;
+  SourceLocation getInsertionPoint() const {
----------------
llvm::DenseSet<const Stmt*>

std::set is a pretty terrible data structure unless you really really need order.
(Lots of allocations, lots of pointer-chasing)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:238
+  // Convert RootStmt Nodes to Stmts and insert in RootStmts.
+  llvm::transform(ExtZone->Parent->Children,
+                  std::inserter(ExtZone->RootStmts, ExtZone->RootStmts.begin()),
----------------
this is also:
```
for (const SelectionTree::Node *N)
  ExtZone->RootStmts.insert(N->ASTNode.get<Stmt>());
```
which is shorter and (I think) less obfuscated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526