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

[Haojian Wu via Phabricator via cfe-commits]

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