[PATCH] D108320: Add semantic token modifier for non-const reference parameter

[Sam McCall via Phabricator via cfe-commits]

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