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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

looks good with a few nits.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+/// a.h:
+///   void foo() { return ; }
+///
----------------
kadircet wrote:
> hokein wrote:
> > 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?
> yes clang-tidy can generate those fixes but I believe a code-action should not generate code that relies on a clang-tidy check to fix it.
I'm still not convinced that we should fix it in the check. but we don't have to solve it out in this patch, could we remove the inline from the sample? (adding it now seems to confuse users, we don't support it yet).


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:154
+// exception of template specialization. For those we return the previous
+// declaration instead of the first one, since it will be template decl itself
+// rather then declaration of specialization.
----------------
nit: the comment is out of date with the new change?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:50
 
+  llvm::StringMap<std::string> ExtraFiles;
+
----------------
nit: add a comment `// file name => file content`.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:712
+      bar();
+    })cpp");
+
----------------
kadircet wrote:
> hokein wrote:
> > 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 }"
> isn't it covered by the next testcase ?
ah, sorry,  I missed that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433