[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:78
+// function decl. Skips local symbols.
+llvm::DenseSet<const Decl *> getNonLocalDeclRefs(const FunctionDecl *FD,
+                                                 ParsedAST &AST) {
----------------
hokein wrote:
> thinking this a bit more, the function is non-trivial, we probably want unit test for it.
agreed, sent out D67748


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:113
+// Checks the decls mentioned in Source are visible in the context of Target.
+// Achives that by performing lookups in Target's DeclContext.
+// Unfortunately these results need to be post-filtered since the AST we get is
----------------
hokein wrote:
> hmm, looks like we don't use TargetContext in the implemenation at all. Is the comment out-of-date now?
we are still checking its visibility in target context, but without making use of declcontext. Updating the comment.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:160
+// usually be the first declaration in current translation unit with the
+// exception of template specialization. For those we return the previous
+// declaration instead of the first one, since it will be template decl itself
----------------
hokein wrote:
> function template is tricky, I may be not aware of the context, what's our plan for supporting it? could you give an concrete example? it seems that the current unit test doesn't cover it.
Yes we plan to support them, added a test case thanks for pointing it out.

This basically ensures that we select the correct decl in the case of template spec decls, because canonical decl is function template itself, instead of the specialization decl.


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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:224
+
+  Expected<Effect> apply(const Selection &Sel) override {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
----------------
hokein wrote:
> The tweak is under development, do we need to hide it until it is completed?
I wasn't planning to land it before all of its dependencies are LGTM'd, but marking it as hidden just in case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433