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