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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks -- this patch is looking great so far!



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:314
 //   (these tend to be vague, like Type or Unknown)
+// - Resolved tokens (i.e. without the "dependent-name" modifier) with kind
+//   "Unknown" are less reliable than resolved tokens with other kinds
----------------
We should consider the case where a dependent name is passed by non-const reference, for example:

```
void increment_counter(int&);

template <typename T>
void bar() {
   increment_counter(T::static_counter);
}
```

This case does not work yet with the current patch (the dependent name is a `DependentScopeDeclRefExpr` rather than a `DeclRefExpr`), but we'll want to make it work in the future.

With the conflict resolution logic in this patch, the `Unknown` token kind from `highlightPassedByNonConstReference()` will be chosen over the dependent token kind.

As it happens, the dependent token kind for expressions is also `Unknown` so it doesn't matter, but perhaps we shouldn't be relying on this. Perhaps the following would make more sense:

1. a token with `Unknown` as the kind has the lowest priority
2. then a token with the `DependentName` modifier (next lowest)
3. then everything else?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:515
+    // Highlighting parameters passed by non-const reference does not really
+    // make sence for these
+    if (isa<CXXOperatorCallExpr>(E) || isa<UserDefinedLiteral>(E))
----------------
nit: sense


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:530
+      const FunctionDecl *FD,
+      llvm::function_ref<Expr *(size_t)> GetArgExprAtIndex) {
+    if (!FD)
----------------
We can avoid the callback by constructing an ArrayRef of the arguments, see a similar case here:

https://searchfox.org/llvm/source/clang-tools-extra/clangd/InlayHints.cpp#56
https://searchfox.org/llvm/source/clang-tools-extra/clangd/InlayHints.cpp#79


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:542
+        // Is this parameter passed by non-const reference?
+        if (T->isLValueReferenceType() && !isConst(T)) {
+          if (auto *Arg = GetArgExprAtIndex(I)) {
----------------
I think there are some edge cases where `isConst()` doesn't do what we want.

For example, I think for a parameter of type `const int*&`, it will return `true` (and thus we will **not** highlight the corresponding argument), even thus this is actually a non-const reference.

So, we may want to use a dedicated function that specifically handles reference-related logic only.

(This probably also makes a good test case.)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:547
+            if (isa<DeclRefExpr>(Arg)) {
+              Location = Arg->getBeginLoc();
+            } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
----------------
For a qualified name (e.g. `A::B`), I think this is going to return the beginning of the qualifier, whereas we only want to highlight the last name (otherwise there won't be a matching token from the first pass).

So I think we want `getLocation()` instead.

(Also makes a good test case.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108320