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