[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:59
+// different location. Contains both function signature and body.
+llvm::Optional<llvm::StringRef> moveFunctionDef(const FunctionDecl *FD) {
+  auto &SM = FD->getASTContext().getSourceManager();
----------------
the function name doesn't reflect what it does, it doesn't move the function, it just returns the source code of the function, I'd call it some name like `getFunctionSourceCode`.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:117
+    const SourceManager &SM = Sel.AST.getSourceManager();
+    llvm::StringRef FileName = SM.getFilename(Sel.Cursor);
+
----------------
The FileName is not always absolute, `getCorrespondingHeaderOrSource` is expecting an absolute file path input.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:119
+
+    auto SourceFile = getSourceFile(FileName, Sel);
+    if (!SourceFile)
----------------
could we use a better name, `Source` in the context here has two different meaning: 1) the corresponding .cc file 2) the source of moving function declaration (we called it `Source`);

Maybe call it `CCFile`?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:157
+        SMFF.get().getComposedLoc(SMFF.get().getMainFileID(), *InsertionOffset);
+    const tooling::Replacement InsertFunctionDef(SMFF.get(), InsertLoc, 0,
+                                                 *FuncDef);
----------------
maybe just `const tooling::Replacement InsertFunctionDef(Contents, *InsertionOffset, 0, *FunDef);`, which would save us a `InsertLoc`.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:166
+        Sel.AST.getSourceManager(),
+        CharSourceRange(Source->getBody()->getSourceRange(), /*ITR=*/true),
+        ";");
----------------
nit: I think `CharSourceRange::getCharRange(Source->getBody()->getSourceRange())` is clearer.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1574
+  ExtraFiles["Test.cpp"] = "";
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,
----------------
thinking more about this,  if this function is inline, we will leave an unnecessary `inline` keyword after running the code action. No need to address it in the patch, just keep in mind.

can we add more tests? e.g. template functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298