[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this, it looks great!

A few comments on:

- a few on cases that I think aren't handled quite right
- structure of the new code
- structure of the *old* code: names etc that no longer fit after these changes

There's also the ugliness around merging edits/tweak effects - this has come up in a couple of other checks and we should have a nicer way to do it, sorry it's not there yet :-)



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:398
 
+std::string NewFunction::renderAttributes() const {
+  std::string Attributes;
----------------
nit: these are *specifiers* rather than attributes


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:412
+
+std::string NewFunction::renderAttributesAfter() const {
+  std::string Attributes;
----------------
and these are *qualifiers* rather than attributes


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:422
+
+std::string NewFunction::renderNamespaceAndClass() const {
+  std::string NamespaceClass;
----------------
this isn't actually rendering namespaces, right? just classes


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:426
+  if (ParentContext) {
+    NamespaceClass = ParentContext.getValue()->getNameAsString();
+    NamespaceClass += "::";
----------------
What if this class is nested?
e.g.

```
class Foo { class Bar { int baz(); }; };
int Foo::Bar::baz() { ... }
```

we need to produce `void Foo::Bar::extracted() { ... }`

I think the easiest thing here is just to save the NestedNameSpecifier* from the original out-of-line definition (getQualifier()), this represents the syntax `Foo::Bar::` as written. Then you can just print() it.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:440
 
+std::string NewFunction::renderInlineDefinition(const SourceManager &SM) const {
+  return std::string(
----------------
the naming of this family of functions, and the irregular reuse between them doesn't sit quite right with me, especially the way it suggests "inline definition" is a special case (when for plain functions it's the only case).

What about `renderDeclaration(FunctionDeclKind K, DeclContext &Enclosing, const SourceManager &SM);`

with `enum FunctionDeclKind { InlineDefinition, ForwardDeclaration, OutOfLineDefinition }`?, 


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:447
+  return std::string(llvm::formatv("{0} {1}{2}({3}){4} {\n{5}\n}\n",
+                                   printType(ReturnType, *EnclosingFuncContext),
+                                   renderNamespaceAndClass(), Name,
----------------
here you're excluding the specifiers, which is correct for `static` but incorrect for `constexpr` :-(


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:455
+  return std::string(llvm::formatv("{0}{1} {2}({3}){4}", renderAttributes(),
+                                   printType(ReturnType, *EnclosingFuncContext),
+                                   Name, renderParametersForDefinition(),
----------------
For out-of-line defined methods, this is the wrong DC, which may lead to the type being incorrectly qualified. The DC should rather be the enclosing class.

Similarly, renderParametersForDefinition uses EnclosingFuncContext too - instead it should take the EnclosingFuncContext as a parameter. (And be renamed `renderParametersForDeclaration`, since it's no longer just for definitions)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:730
+
+  if (isa<CXXMethodDecl>(ExtZone.EnclosingFunction)) {
+    const auto *Method =
----------------
Maybe a high-level comment:

For methods defined out-of-line, the extracted method will also be out-of-line. The new declaration/definition will each be next to the existing ones.
Methods defined inline are treated very similarly to plain functions.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:737
+    ExtractedFunc.Static = Method->isStatic();
+    ExtractedFunc.Constexpr = Method->isConstexpr();
+    ExtractedFunc.ContextConst = Method->isConst();
----------------
may want to propagate consteval too
maybe instead of `bool Constexpr`, store `ConstexprSpecKind Constexpr = Method->getConstexprKind()`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:738
+    ExtractedFunc.Constexpr = Method->isConstexpr();
+    ExtractedFunc.ContextConst = Method->isConst();
+
----------------
nit: why is this called ContextConst rather than just const? We can already decide at this point that the new function will be const.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:743
+
+    ExtractedFunc.ParentContext = Method->getParent();
+    ExtractedFunc.DeclarationPoint = DeclPos.getValue();
----------------
I think we should call this EnclosingClass rather than ParentContext, as we only set it/care about it when it's a class, never when it's a namespace


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:744
+    ExtractedFunc.ParentContext = Method->getParent();
+    ExtractedFunc.DeclarationPoint = DeclPos.getValue();
+  }
----------------
There's some redundancy between DeclarationPoint and OutOfLine.

Can we call this one ForwardDeclarationPoint, only set it if the method is out-of-line? Then we can get rid of OutOfLine and just check the presence/absence of ForwardDeclarationPoint.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:744
+    ExtractedFunc.ParentContext = Method->getParent();
+    ExtractedFunc.DeclarationPoint = DeclPos.getValue();
+  }
----------------
sammccall wrote:
> There's some redundancy between DeclarationPoint and OutOfLine.
> 
> Can we call this one ForwardDeclarationPoint, only set it if the method is out-of-line? Then we can get rid of OutOfLine and just check the presence/absence of ForwardDeclarationPoint.
it might be more appropriate to place this elsewhere in the class, e.g. in the private section, among the methods even if we're extracting from a constructor etc. We now have clangd/refactor/InsertionPoint.h to help with this.

I don't think you should do this now (keep the scope small), but might be worth a FIXME


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:748
   ExtractedFunc.BodyRange = ExtZone.ZoneRange;
   ExtractedFunc.InsertionPoint = ExtZone.getInsertionPoint();
   ExtractedFunc.EnclosingFuncContext =
----------------
now that we have multiple insertions, NewFunction::InsertionPoint should be called DefinitionPoint instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122698