tom-anders marked 5 inline comments as done. tom-anders added inline comments.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:470 std::vector<HighlightingToken> Tokens; + std::map<Range, std::set<HighlightingModifier>> ExtraModifiers; const HeuristicResolver *Resolver; ---------------- 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. ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:773 + )cpp", + /*struct $Class_decl[[S]] { + $Class_decl[[S]](int $Parameter_decl[[value]], const int& $Parameter_decl_readonly[[constRef]], ---------------- nridge wrote: > (test case needs uncommenting) Actually it needed to be removed, has been replaced by the test above 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
