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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
