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