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

[Tom Praschan via Phabricator via cfe-commits]

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