[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

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