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

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

malaperle marked an inline comment as done.
malaperle added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:113
+      auto KindVal = static_cast<SymKindUnderlyingType>(Kind);
+      if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax)
+        SupportedSymbolKinds.set(KindVal);
----------------
sammccall wrote:
> 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?
I think it's better to keep vector<SymbolKind> in Protocol.h, it is clearer and more in line with the protocol. I overloaded fromJSON, which simplifies the code here, but it still needs a static_cast to size_t for the bitset.set(). Other places there still needs a static_cast, like in defaultSymbolKinds for the loop, I can static_cast to size_t everywhere (or int?) but having SymKindUnderlyingType seems more correct. I changed it to size_t, let me know if it was something like that you had in mind.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882