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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
