sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry about the delay Marc-André - I got stuck on "how to test ClangdLSPServer" 
and then distracted!
But this looks great.

Comment at: clangd/ClangdLSPServer.cpp:101
+  // All clients should support those.
+  for (SymKindUnderlyingType I = SymbolKindMin;
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
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 

Comment at: clangd/ClangdLSPServer.cpp:103
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
+    SupportedSymbolKinds.set(I);
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.

Comment at: clangd/ClangdServer.h:164
+  void workspaceSymbols(StringRef Query,
+                        const clangd::WorkspaceSymbolOptions &Opts,
+                        const DraftStore &DS,
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.

Comment at: clangd/ClangdServer.h:165
+                        const clangd::WorkspaceSymbolOptions &Opts,
+                        const DraftStore &DS,
+                        Callback<std::vector<SymbolInformation>> CB);
maybe a short FIXME here too e.g. "remove param when the index has line/col"

Comment at: clangd/FindSymbols.cpp:42
+  if (!supportedSymbolKinds) {
+    // Provide some sensible default when all fails.
malaperle wrote:
> sammccall wrote:
> > This code shouldn't need to handle this case. The LSP specifies the default 
> > set of supported types if it's not provided, so ClangdLSPServer should 
> > enure we always have a valid set
> My thought is that if we start dealing with one of those:
> ```
>   Object = 19,
>   Key = 20,
>   Null = 21,
>   Event = 24,
>   Operator = 25,
>   TypeParameter = 26
> ```
> and it's an older client that only supports up to "Array = 18", then we can 
> provide a fallback, otherwise the client my not handle the unknown kind 
> gracefully. But we can add this later if necessary I guess.
Ah yes I agree, fallbacks are good. I just meant that ClangdLSPServer should 
ensure that we actually have the bitset populated with the right values. I 
think it looks good now.

Comment at: clangd/Protocol.cpp:209
+  default:
+    llvm_unreachable("Unexpected symbol kind");
+  }
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...)

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to