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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:332
              }},
+            {"declarationProvider", true},
             {"definitionProvider", true},
----------------
hokein wrote:
> I believe missing this is a LSP protocol bug, vscode uses this term https://github.com/Microsoft/vscode/blob/248aea7fd51a703a7ab94ae5a582c85c11fbff33/src/vs/vscode.d.ts#L2301.
Yeah, agree.


================
Comment at: clangd/XRefs.cpp:38
+  // Only a single declaration is allowed.
+  if (isa<ValueDecl>(D)) // except cases above
+    return D;
----------------
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).



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57388