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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
