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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Still LG, thanks!
I'll look into the testing issue.



================
Comment at: clangd/ClangdLSPServer.cpp:113
+      auto KindVal = static_cast<SymKindUnderlyingType>(Kind);
+      if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax)
+        SupportedSymbolKinds.set(KindVal);
----------------
nit: the bounds checks at usage types, with explicit underlying type for the enum are inconsistent with what we do in other protocol enums, and seem to clutter the code.

All else equal, I'd prefer to have an enum/enum class with implicit type, and not have values that are out of the enum's range. It's a simpler model that matches the code we have, and doesn't need tricky casts to SymKindUnderlyingType. If we want to support clients that send higher numbers than we know about, we could either:
 - overload fromJSON(vector<SymbolKind>)&
 - just express the capabilities as vector<int> and convert them to the enum (and check bounds) at this point.
WDYT?


================
Comment at: clangd/FindSymbols.h:26
+
+llvm::Expected<std::vector<SymbolInformation>> getWorkspaceSymbols(
+    llvm::StringRef Query, int Limit, const SymbolIndex *const Index,
----------------
Would be nice to have a comment describing: query syntax, limit=0 special behavior.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882