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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
