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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+/// a.h:
+///   void foo() { return ; }
+///
----------------
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.


================
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();
+  }
----------------
hokein wrote:
> 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.
theoretically, a template specialization cannot be the first decl, as one needs to declare function template first.
making this more clear by changing this into a loop that finds the first forward declaration for the specialization.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:712
+      bar();
+    })cpp");
+
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433