[PATCH] D44882: [clangd] Implementation of workspace/symbol request

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

In https://reviews.llvm.org/D44882#1064196, @sammccall wrote:

> (BTW @hokein has https://reviews.llvm.org/D45513 to add row/col to the index, which will allow all the file-reading stuff to be cleaned up, but no need to wait on that since it's all working now).


Looking forward to it!



================
Comment at: clangd/ClangdLSPServer.cpp:101
+  // All clients should support those.
+  for (SymKindUnderlyingType I = SymbolKindMin;
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
----------------
sammccall wrote:
> malaperle wrote:
> > I'd like to add some test to exercise the changes in ClangdLSPServer.cpp and Protocol.cpp but it's not straightforward to do nicely. Using a "lit" test, I cannot have a header and since symbols in the main file are not collected then it's not useful. If I make a gtest, I have to feed it the lsp server with stdin and check the stdout which is a bit messy, but doable.
> Yeah, we don't have a good way to test ClangdLSPServer :-( I would like to fix that, but I don't know of any easy fixes.
> 
> This is kind of gross, but do standard library includes work from our lit tests? You could `#include <map>` and then test using some symbols you know are there...
> 
It doesn't seem to work unfortunately. I'm not sure why yet. Either way, I think I would add this test as a separate patch if we are to add a standard library include because I'm a bit nervous it will break and we will have to revert it :)


================
Comment at: clangd/ClangdLSPServer.cpp:103
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
+    SupportedSymbolKinds.set(I);
+
----------------
sammccall wrote:
> I'd like to be slightly less hostile than this to (broken) clients that fail to call initialize.
> As the patch stands they'll get an empty SupportedSymbolKinds, and we'll crash if they call documentSymbols.
> 
> Instead I'd suggest pulling out defaultSymbolKinds() and initializing to that in the constructor, and then overriding with either `SpecifiedSymbolKinds` or `SpecifiedSymbolKinds | defaultSymbolKinds()` here.
Good catch. I initialized SupportedSymbolKinds with defaultSymbolKinds() and let this loop add additional symbol kinds.


================
Comment at: clangd/ClangdServer.h:164
+  void workspaceSymbols(StringRef Query,
+                        const clangd::WorkspaceSymbolOptions &Opts,
+                        const DraftStore &DS,
----------------
sammccall wrote:
> I think it would probably be more consistent with other functions to just take `int limit` here. I'm not sure CodeCompletion is an example we want to emulate.
> 
> ClangdLSPServer might even just grab it from the code completion options, since that's the behavior we actually want.
> 
> Up to you, though.
I think it makes sense. We can reintroduce an option struct when there is more than one thing in it.


================
Comment at: clangd/Protocol.cpp:209
+  default:
+    llvm_unreachable("Unexpected symbol kind");
+  }
----------------
sammccall wrote:
> This doesn't actually seem unreachable (or won't stay that way), maybe log and return `Null` or something like that? (Wow, there's really no catch-all option for this mandatory enum...)
`Null` Is not in LSP 1.0 and 2.0 that's why I had put `Variable` at the beginning. Maybe `String` is better. Not sure what you think is best.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882