kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:29
+// criteria is met.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+ if (!SelNode)
----------------
hokein wrote:
> nit: looks like we also a similar version in `DefineInline`? would be nice if
> we could share the implementation. I don't have a good idea where to put it,
> maybe add a FIXME?
I also had the same concern, but left it here since couldn't find a suitable
please.
adding a fixme.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:44
+/// Moves definition of a function/method to an appropriate implementation
file.
+/// If current file is already an implementation file, tries to move the
+/// definition out-of-line.
----------------
hokein wrote:
> I'm not sure this is useful in general: saying in .cc we have a single
> definition `void foo() {}`, after running the code action, we will end up
> with `void foo(); void foo() {}`.
>
> If we want to do this, I think we may make it only available for inline class
> methods.
yes this was implying methods in my head, but forgot to spell it out. updating
comment.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:75
+
+ // Bail out if we are not in a header file.
+ // FIXME: We might want to consider moving method definitions below class
----------------
hokein wrote:
> The bail-out here seems violating the above comment `/// If current file is
> already an implementation file, tries to move the definition out-of-line`.
> Basically, now we only allow moving the function definition to a .cc file.
this was actually representing the state I wanted to arrive, but you are right
it looks confusing, getting rid of the mention in the class documention.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69266/new/
https://reviews.llvm.org/D69266
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
