ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:389
llvm::sort(References, [](const Reference &L, const Reference &R) {
- return std::tie(L.Loc, L.CanonicalTarget, L.Role) <
- std::tie(R.Loc, R.CanonicalTarget, R.Role);
+ return L.Loc < R.Loc;
});
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > hokein wrote:
> > > > > ilya-biryukov wrote:
> > > > > > hokein wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > What are the elements `References` for the problematic case?
> > > > > > > >
> > > > > > > > If we have duplicate elements, then `sort()` would now give us
> > > > > > > > one of the items. Which exact `Decl` we're gonna end up seeing
> > > > > > > > is not well-defined, i.e. it's non-deterministic.
> > > > > > > > What are the elements References for the problematic case?
> > > > > > >
> > > > > > > The testcase is using declarations (see the existing test) -- we
> > > > > > > will get 3 refs on `using ::fo^o`, each ref has a different decl.
> > > > > > >
> > > > > > >
> > > > > > > ```
> > > > > > > void [[foo]](int);
> > > > > > > void [[foo]](double);
> > > > > > >
> > > > > > > namespace ns {
> > > > > > > using ::[[fo^o]];
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > > If we have duplicate elements, then sort() would now give us
> > > > > > > > one of the items. Which exact Decl we're gonna end up seeing is
> > > > > > > > not well-defined, i.e. it's non-deterministic.
> > > > > > >
> > > > > > > I think we could make it deterministic, but only return one refs,
> > > > > > > WDYT?
> > > > > > >
> > > > > > >
> > > > > > To make sure I understand the problem: we used to get 4 references
> > > > > > in total:
> > > > > > - 2 references to the functions (`foo(int)` and `foo(double)`)
> > > > > > - 2 references pointing to the using declaration, e.g. the `using
> > > > > > ::[foo]]`
> > > > > >
> > > > > > After this patch we would remove the duplicate `using ::[[foo]]`
> > > > > > from the results and get only 3 references as a result.
> > > > > >
> > > > > > Is that correct?
> > > > > Yes, that is correct.
> > > > Interestingly enough, we always ignore the `CanonicalTarget` in the
> > > > returned `Reference`.
> > > >
> > > > Maybe we could remove the `CanonicalTarget` from the `Reference` struct
> > > > instead?
> > > > That keeps the interface more consistent: clients will always get
> > > > deterministic results (as there is no `CanonicalDecl`, which is
> > > > different now)
> > > > Maybe we could remove the CanonicalTarget from the Reference struct
> > > > instead?
> > >
> > > unfortunately, this may not work either because of the `Role` -- we still
> > > fail on the above sample, the references pointing to the using decl have
> > > different roles.
> > We only look at the role in `DocumentHighlights` to determine the
> > `DocumentHighlightKind` (`Read`, `Write` or `Text`).
> > I suggest we do this earlier and replace the `Reference::Role` field with
> > `Reference::DocumentHighlightKind`.
> > Then we can de-duplicate here and keep deterministic interface.
> >
> > If we feel that's the wrong layering (we are putting LSP-specific things
> > into Reference, which only has clang-specific types stuff now), we could
> > move de-duplication downstream to `findDocumentHighlights` and `findRefs`.
> > They return `Loc` and `DocumentHighlighting` and can safely de-duplicate on
> > those without changing observable results.
> >
> > Looks like replacing `Role` with `HighlightingKind` is the simplest option,
> > though. And I don't think this breaks layering that much, it's an
> > implementation detail of cross-references anyway.
> > Looks like replacing Role with HighlightingKind is the simplest option,
> > though. And I don't think this breaks layering that much, it's an
> > implementation detail of cross-references anyway.
>
> However, we may still encounter cases where we have duplicated `Loc`s with
> different `HighlightingKind`s.
>
> This leaves us two choices:
> 1) de-duplicate the refs only by loc (as the original implemenation of the
> patch)
> 2) keep Role in refs, and do deduplication on both `findDocumentHighlights`
> and `findRefs`
>
> I personally prefer 1). For document highlights, I'm not sure whether we
> should return multiple highlights on the same location with different
> `HighlightingKind`s, I think most of clients just choose one to render, so
> making clangd return only one result seems reasonable.
I would vouch for (2):
- It's easy to explain why we de-duplicate same locations in `findRefs` as this
is what it is exactly the return value.
- The decision about picking the kind for a duplicated location falls onto
`findDocumentHighlights`, which is, again, the right place to make this
decision (even if the decision is random, e.g. preferring `Write` over `Read`).
But if we really want to keep the code shared for both versions, let's define
how we pick the winner in the case where location is the same.
The important bit is avoid non-deterministic in the observable results of this
function. We should avoid `Decl*` or `Role` being part of the result and being
picked randomly from a few options that we have.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66349/new/
https://reviews.llvm.org/D66349
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
