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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the update! Looks pretty good, just a few minor comments from me. Will leave approval to Sam.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:470
   std::vector<HighlightingToken> Tokens;
+  std::map<Range, std::set<HighlightingModifier>> ExtraModifiers;
   const HeuristicResolver *Resolver;
----------------
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


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:553
+    if (auto *ProtoType = FD->getType()->getAs<FunctionProtoType>()) {
+        // Iterate over the declarations of the function parameters.
+        // If any of them are non-const reference paramters, add it as a
----------------
nit: declarations --> types


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:554
+        // Iterate over the declarations of the function parameters.
+        // If any of them are non-const reference paramters, add it as a
+        // highlighting modifier to the corresponding expression
----------------
nit: parameters


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:560
+            // Is this parameter passed by non-const reference?
+            if (T->isLValueReferenceType() && !T.getNonReferenceType().isConstQualified() && !T->isDependentType()) {
+              if (auto *Arg = Args[I]) {
----------------
I think we can relax the dependent type check here a bit, for example `std::vector<T>&` is a dependent type but it's definitely a mutable reference.

(Feel free to leave that for later and just add a FIXME.)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:566
+                // e.g. highlight `a` in `a[i]`
+                if (auto *DR = dyn_cast<DeclRefExpr>(Arg)) {
+                  Location = DR->getLocation();
----------------
Maybe also add a `// FIXME: Handle dependent expression types` just so we don't forget


================
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]], 
----------------
(test case needs uncommenting)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108320/new/

https://reviews.llvm.org/D108320