malaperle added a comment.

In, @sammccall wrote:

> (BTW @hokein has 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.

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to