sammccall added a comment.
Agree this is nice, well done! A few more notes for consideration...
================
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
----------------
nridge wrote:
> 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?
The conflict-resolution idea is subtle (and IME hard to debug). I'm wary of
overloading it by deliberately introducing "conflicts" that should actually be
merged. Did you consider the idea of tracking extra modifiers separately and
merging them in at the end?
---
BTW: we're stretching the meaning of `Unknown` here. There are two subtly
different concepts:
- clangd happens not to have determined the kind of this token, e.g. because
we missed a case (uses in this patch)
- clangd has determined that per C++ rules the kind of token is ambiguous
(uses prior to this patch)
Call me weird, but I have "Unknown" highlighted in bright orange in my editor,
because I want to know about the second case :-)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:514
+ bool VisitCallExpr(CallExpr *E) {
+ // Highlighting parameters passed by non-const reference does not really
+ // make sence for these
----------------
CXXOperatorCallExpr seems to make sense to me for the most part:
- functor calls are CXXOperatorCallExpr
- if `x + y` mutates y, I want to know that
There are some exceptions though:
- the functor object itself is more like the function callee, and to be
consistent we shouldn't highlight it
- highlighting `x` in `x += y` is weird
- `a << b` is a weird ambiguous case ("stream" vs "shift" want different
behavior)
I think these can be handled by choosing a minimum argument index to highlight
based on the operator type.
I think it makes sense to leave operators out of scope for this patch, but IMO
should be a "FIXME" rather than a "let's never do this" :-)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:528
+
+ void highlightPassedByNonConstReference(
+ const FunctionDecl *FD,
----------------
nit: this function name is a bit hard to parse, "highlight" is a transitive
verb but the rest of the name isn't its object.
Maybe `highlightMutableReferenceArguments`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:538
+ for (size_t I = 0; I < FD->getNumParams(); ++I) {
+ if (const auto *Param = FD->getParamDecl(I)) {
+ auto T = Param->getType();
----------------
I feel like you'd be better off using the FunctionProtoType and iterating over
argument types, rather than the argument declarations on a particular
declaration of the function.
e.g. this code is legal in C:
```
int x(); // i suspect this is the canonical decl
int x(int); // but this one provides the type
```
We don't have references in C of course!, but maybe similar issues lurking...
================
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)) {
----------------
nridge wrote:
> 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.)
Yeah this is the core of this modifier, worth being precise and explicit here.
I think we want to match only reference types where the inner type is not
top-level const.
I think we should also conservatively forbid the inner type from being
*dependent*. Consider the following function:
```
template <typename X>
void foo(X& x) {
foo(x); // this call
}
```
Locally, the recursive call looks like "mutable reference to dependent type".
But when instantiated, X may be `const int` and is in fact very likely to be
deduced that way in practice.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:546
+
+ if (isa<DeclRefExpr>(Arg)) {
+ Location = Arg->getBeginLoc();
----------------
You may want to add an "unwrapping" step so that e.g. ArraySubscriptExpr `a[i]`
can be unwrapped to `a` and then possibly highlighted (and its
operator-overloaded form).
Wouldn't suggest doing that in this patch, but you could leave a note if you
like.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:547
+ if (isa<DeclRefExpr>(Arg)) {
+ Location = Arg->getBeginLoc();
+ } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
----------------
nridge wrote:
> 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.)
And getLocation() will do the right thing for DeclRefExpr, MemberExpr, and
others, so this can just be `isa<DeclRefExpr, MemberExpr>` with no need for
dyn_cast.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:72
DefaultLibrary,
+ PassedByNonConstRef,
----------------
nit: no line break here. The line breaks separate normal modifiers vs scope
modifiers (which are mutually exclusive) vs sentinels
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:73
+ PassedByNonConstRef,
+
----------------
nit: "mutable" is more natural than "non-const", and modifiers don't use
heavily C++-specific names anyway
Please spell out "reference", we don't use other abbreviations.
I'd consider "UsedAsMutableReference" rather than "passed" as this seems to be
the deeper semantic idea we're expressing, and we may want to generalize this
in some way
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
