tom-anders marked 12 inline comments as done.
tom-anders added inline comments.
================
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:
> sammccall wrote:
> > 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 :-)
> I don't have a strong opinion on the options here, just wanted to chime in
> and say I also highlight `Unknown` prominently for similar reasons :)
I switched to @sammccall's idea of having a map<Range, Modifier>, I agree that
it feels more natural than the previous version
================
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
----------------
sammccall wrote:
> 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" :-)
Agreed, added a FIXME
================
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)) {
----------------
sammccall wrote:
> 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.
I adjusted the condition accordingly and added a few more test cases (including
stuff like `const int*&`).
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:546
+
+ if (isa<DeclRefExpr>(Arg)) {
+ Location = Arg->getBeginLoc();
----------------
sammccall wrote:
> 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.
Added a FIXME.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:547
+ if (isa<DeclRefExpr>(Arg)) {
+ Location = Arg->getBeginLoc();
+ } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
----------------
tom-anders wrote:
> sammccall wrote:
> > tom-anders wrote:
> > > sammccall wrote:
> > > > 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.
> > > I'm not sure which getLocation() you're talking about here. There's
> > > DeclRefExpr::getLocation(), but neither Expr::getLocation() nor
> > > MemberExpr::getLocation(). Am I missing something?
> > No, I think I'm just going mad (I was thinking of Decl::getLocation I
> > guess).
> > Never mind and sorry!
> np :D
Switched to `getLocation()` and added a 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
