sammccall accepted this revision. sammccall added a comment. Still LG
================ Comment at: clangd/ClangdLSPServer.cpp:113 + auto KindVal = static_cast<SymKindUnderlyingType>(Kind); + if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax) + SupportedSymbolKinds.set(KindVal); ---------------- malaperle wrote: > 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. LG, though I've commented in one place where the cast seems necessary. Personally I usually use int here, but size_t is good too. The exact type doesn't matter as long as it covers all values of the enum. ================ Comment at: clangd/ClangdLSPServer.cpp:274 + Sym.kind = adjustKindToCapability(Sym.kind, SupportedSymbolKinds); + assert(static_cast<size_t>(Sym.kind) >= SymbolKindMin && + static_cast<size_t>(Sym.kind) < SupportedSymbolKinds.size() && ---------------- you no longer need the min/max asserts and their casts, because we don't allow any values of type SymbolKind that don't have one of the enum values (i.e. we're just echoing the type system here) 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
