[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

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