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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, I think the main remaining thing is enclosing contexts... (sorry, I wasn't thinking clearly about these in the last round, and I think existing code gets this slightly wrong too).

Each declaration has potentially *two* contexts, semantic and syntactic, and the distinction is relevant here.

  namespace ns {
  class A {
    void f(); // SemanticDC = A, SyntacticDC = A
  };
  void A::f() { } // SemanticDC = A, SyntacticDC = ns
  }

To determine how to spell different parts of the function, different rules apply to different parts of the declaration:

- return type and function name needs to be relative to the SyntacticDC
- parameters and function body are relative to the SemanticDC

I think `renderDeclaration()` should take SemanticDC and SyntacticDC as parameters, and NewFunction should have SemanticDC, SyntacticDC, ForwardDeclarationSyntacticDC as fields. (By definition, the forward declaration and the definition have the same semantic DC).

To summarize, something like:

  struct NewFunction {
    DeclContext *SemanticDC;
    DeclContext *SyntacticDC;
    DeclContext *ForwardDeclarationSyntacticDC;
  }
  void renderDeclaration(FunctionDeclKind, DeclContext& SemanticDC, DeclContext &SyntacticDC, ...) {
    ... printType(ReturnType, SyntacticDC), ..., renderParametersForDefinition(SemanticDC)...
  }
  void createFunctionDefinition(...) {
     renderDeclaration(..., SemanticDC, SyntacticDC, ...);
  }
  void createForwardDeclaration(...) {
    renderDeclaration(..., SemanticDC, ForwardDeclarationSyntacticDC, ...);
  }



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:352
+  SourceLocation DefinitionPoint;
+  llvm::Optional<SourceRange> ForwardDeclarationPoint;
+  const CXXRecordDecl *EnclosingClass = nullptr;
----------------
you only ever use the beginning of this range (and it's an insertion point, so having a range doesn't make much sense). Change to SourceLocation?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:354
+  const CXXRecordDecl *EnclosingClass = nullptr;
+  NestedNameSpecifier *NestedNameSpec = nullptr;
+  const DeclContext *EnclosingFuncContext = nullptr;
----------------
nit: const (the AST isn't great about const-correctness, but should be OK here)

Maybe NestedNameSpec -> DefinitionQualifier?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:361
+
+  const DeclContext &getEnclosing() const;
+
----------------
this needs a more specific name - see overall comment on the patch


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:366
   tooling::ExtractionSemicolonPolicy SemicolonPolicy;
-  NewFunction(tooling::ExtractionSemicolonPolicy SemicolonPolicy)
-      : SemicolonPolicy(SemicolonPolicy) {}
+  const LangOptions &LangOpts;
+  NewFunction(tooling::ExtractionSemicolonPolicy SemicolonPolicy,
----------------
nit: pointer rather than reference, reference members have surprising effects and we don't have any here so far


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:473
 
-std::string NewFunction::renderDefinition(const SourceManager &SM) const {
+std::string NewFunction::renderDefinition(FunctionDeclKind K,
+                                          const DeclContext &Enclosing,
----------------
you don't need separate renderDeclaration() and renderDefinition() functions: the FunctionDeclKind tells you whether this is a definition or not.

(And naming-wise, definitions are just a special type of declaration)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:847
+
+tooling::Replacement createFunctionDeclaration(const NewFunction &ExtractedFunc,
+                                               const SourceManager &SM) {
----------------
nit: createForwardDeclaration


================
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(),
----------------
FabioRS wrote:
> sammccall wrote:
> > 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)
> It is only for out-of-line methods? I put this verification in the getEnclosing() of the new diff.
> 
functions can also be defined in a different syntactic context than they are declared:
```
namespace ns { void foo(); }
void ns::foo() {}
```

This is rarer and I suspect we get this case wrong today.


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

https://reviews.llvm.org/D122698