hokein marked an inline comment as done.
hokein 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;
});
----------------
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.
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
