[PATCH] D65433: [clangd] DefineInline action availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

mostly good to me, a few more comments.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+/// a.h:
+///   void foo() { return ; }
+///
----------------
kadircet wrote:
> hokein wrote:
> > now we get a potential ODR violation in this example, maybe choose a different example?
> You are right, but we cannot get away from it by changing the example. Adding an "inline " instead, and I believe that's what we should do if we are moving a function definition to a header.
I think not all cases will need inline, e.g. class method, or function template(?).  Fix these problems is out-scope of the tweak (there is a clang-tidy check handling this case), and probably add complexity to the implementation. I'm leaning on not fixing it.

Maybe a better example is class method?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:156
+// rather then declaration of specialization.
+const FunctionDecl *choseCanonical(const FunctionDecl *FD) {
+  if (FD->isFunctionTemplateSpecialization()) {
----------------
maybe name it `findTarget`? I think the function is used to find a potential target decl which the definition will be moved to,  `Canonical` doesn't provide much information here.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:160
+    // target we want to inline into. Instead we use the previous declaration.
+    return FD->getPreviousDecl();
+  }
----------------
We may get a nullptr if FD is the first declaration, I think we need to handle the nullptr case, otherwise the check will crash.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:185
+  Intent intent() const override { return Intent::Refactor; }
+  std::string title() const override { return "Inline function definition"; }
+  bool hidden() const override { return true; }
----------------
I'm not sure `Inline function definition` is a widely-known refactoring term, I think "Move function definition to declaration" is probably easier for normal C++ developers to understand?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:712
+      bar();
+    })cpp");
+
----------------
I think we missing a basic test where a declaration is in the header and it can be moved?

Header = "void foo();"
Main = "void f^oo() { // no dependency code }"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433