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

Fri Feb 1 02:51:01 PST 2019

sammccall marked 2 inline comments as done.
sammccall added inline comments.

================
Comment at: clangd/XRefs.cpp:38
+  // Only a single declaration is allowed.
+  if (isa<ValueDecl>(D)) // except cases above
+    return D;
----------------
hokein wrote:
> 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?
Added a little bit - there's not really so much to say, because the new behavior is kind of the obvious one.
The old behavior ("just return nullptr if it doesn't matter") maybe deserved a longer comment, but we didn't notice.

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57388