hokein added a comment.
the patch looks mostly good. would be interesting to see `apply` implementation.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:29
+// criteria is met.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+ if (!SelNode)
----------------
nit: looks like we also a similar version in `DefineInline`? would be nice if
we could share the implementation. I don't have a good idea where to put it,
maybe add a FIXME?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:44
+/// Moves definition of a function/method to an appropriate implementation
file.
+/// If current file is already an implementation file, tries to move the
+/// definition out-of-line.
----------------
I'm not sure this is useful in general: saying in .cc we have a single
definition `void foo() {}`, after running the code action, we will end up with
`void foo(); void foo() {}`.
If we want to do this, I think we may make it only available for inline class
methods.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:75
+
+ // Bail out if we are not in a header file.
+ // FIXME: We might want to consider moving method definitions below class
----------------
The bail-out here seems violating the above comment `/// If current file is
already an implementation file, tries to move the definition out-of-line`.
Basically, now we only allow moving the function definition to a .cc file.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:78
+ // definition even if we are inside a source file.
+ auto Type = driver::types::lookupTypeForExtension(
+ llvm::sys::path::extension(FileName).substr(1));
----------------
I think we can use `AST.getASTContext().getLangOpts().IsHeaderFile;` to detect
the header file rather than relying on the filename extension.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1552
+ class Bar {
+ void baz();
+ };
----------------
could you add a testcase where the method is inline? like
```
class Bar {
void baz2() { return; }
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69266/new/
https://reviews.llvm.org/D69266
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
