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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
