[PATCH] D66349: [clangd] Fix one testcase in XRefsTests.

[Haojian Wu via Phabricator via cfe-commits]

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:
> > > > > 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66349/new/

https://reviews.llvm.org/D66349