sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks! This looks good to me to land as is. I'd like to see the std::map eliminated, but if you don't want to do this I'm happy to take a stab at it after. Want someone to commit this for you? ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:470 std::vector<HighlightingToken> Tokens; + std::map<Range, std::set<HighlightingModifier>> ExtraModifiers; const HeuristicResolver *Resolver; ---------------- tom-anders wrote: > nridge wrote: > > Looking at existing usage of associative containers in clangd code, > > `llvm::DenseMap` and `llvm::DenseSet` (which are hashtable-based) are more > > commonly used than `std::map` and `std::set`, though it may require some > > boilerplate to teach them to use new key types > > ([example](https://searchfox.org/llvm/rev/2556f58148836f0af3ad2c73fe54bbdf49f0295a/clang-tools-extra/clangd/index/dex/Token.h#117)) > > > > We could also consider making the value just a vector instead of a set > > (maybe even an `llvm::SmallVector<HighlightingModifier, 1>` given its > > anticipated usage), since even in the unlikely case that we get duplicates, > > `addModifier()` will handle them correctly > Agreed, changed std::set to llvm::SmallVector. I can't really judge if it's > worth using llvm::DenseMap here though. We do try to avoid std::map/std::set as they're pretty gratuitous performance sinks. And the map here may not be tiny. I'd suggest either: - make this a flat `vector<pair<Location, HighlightingModifier>>`, sort it after building, and use binary search to find the right place. This has the same big-O as std::map but without all the allocations and pointer chasing - Specializing DenseMapInfo<Location> in protocol.h and using DenseMap<Location, SmallVector<HighlightingModifier,1>> We don't really need the full Range in the key, as the start defines it. > We could also consider making the value just a vector instead of a set In practice these sets of modifiers end up being a bitmask, so if using a map, you could just store a uint32_t of `1<<Modifier` and add an `addModifiers(uint32_t)` to HighlightingToken. I'm too really worried about preserving that encapsulation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108320/new/ https://reviews.llvm.org/D108320 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
