hokein added a comment.
Looks good overall, most are nits.
> textDocument/definition now returns a definition if one is found, otherwise
> the declaration. It no longer returns declaration + definition if they are
> distinct.
> textDocument/declaration returns the best declaration we can find.
> For macros, the active macro definition is returned for both methods.
> For include directive, the top of the target file is returned for both.
Just realized these at the last minutes, I think these are well-documented (and
very helpful for future-code readers), I'd suggest putting them to the code
comments as well.
================
Comment at: clangd/ClangdLSPServer.cpp:332
}},
+ {"declarationProvider", true},
{"definitionProvider", true},
----------------
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.
================
Comment at: clangd/ClangdLSPServer.h:82
Callback<std::vector<Location>>);
+ void onGoToDeclaration(const TextDocumentPositionParams &,
+ Callback<std::vector<Location>>);
----------------
nit: maybe put it before `onGoToDefinition`, it seems to me more natural that
declaration comes first.
================
Comment at: clangd/ClangdServer.h:167
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
+ void locateSymbolAt(PathRef File, Position Pos,
----------------
nit: this comment seems out-of-date?
================
Comment at: clangd/XRefs.cpp:38
+ // Only a single declaration is allowed.
+ if (isa<ValueDecl>(D)) // except cases above
+ return D;
----------------
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).
================
Comment at: clangd/XRefs.cpp:315
+ // Record SymbolID for index lookup later.
SymbolID Key("");
if (auto ID = getSymbolID(D))
----------------
nit: this is not used anymore.
================
Comment at: clangd/XRefs.h:29
+// - a declaration and a distinct definition (e.g. function declared in
header)
+// - a declaration and an equal definition (e.g. inline function, or class)
+struct LocatedSymbol {
----------------
nit: maybe also mention `marco`? it is unclear to me how the declaration and
definition would be like for macro without reading the implementation.
================
Comment at: unittests/clangd/XRefsTests.cpp:339
+ if (!T.ranges("def").empty())
+ WantDecl = T.range("def");
+
----------------
I think this should be `WantDef`?
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
