[PATCH] D57388: [clangd] Implement textDocument/declaration from LSP 3.14

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/XRefs.cpp:38
+  // Only a single declaration is allowed.
+  if (isa<ValueDecl>(D)) // except cases above
+    return D;
----------------
sammccall wrote:
> hokein wrote:
> > is this a case that we were missing before? can we add a unittest for it (I didn't find a related change in the unittest).
> Previously, this function only had to be correct for things that can be declared and defined separately.
> For some decls that are always definitions (e.g. member variables) we would return nullptr here, and treat them as decl-only ... but that was OK, because the return value was just "a list of locations" and it didn't matter whether we treated them as decls or defs when there's just one.
> 
> However now the return type is "here's the decl, and here's the def". Without this change, we have Definition == llvm::None for e.g. member variables. While the LSP method falls back from def->decl so you probably can't observe this problem, API users can.
> 
> This is in fact covered by the tests, though it's kind of indirect (this is a private helper function, after all).
> 
Thanks for the explanation. Maybe add more in the comment?


================
Comment at: unittests/clangd/XRefsTests.cpp:339
+    if (!T.ranges("def").empty())
+      WantDecl = T.range("def");
+
----------------
hokein wrote:
> I think this should be `WantDef`? 
what about this comment?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57388