ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LG, thanks.
And a question of what are the things we can accidentally misdetect as explicit
================
Comment at: clangd/XRefs.cpp:105
+ // Sort results. Declarations being referenced explicitly come first.
+ std::sort(Result.begin(), Result.end(),
+ [](const DeclInfo &L, const DeclInfo &R) {
----------------
hokein wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Maybe sort further at the callers instead?
> > > It would be a more intrusive change, but would also give a well-defined
> > > result order for findDefinitions and other cases. E.g. findDefinitions
> > > currently gives results in an arbitrary order (specifically, the one
> > > supplied by DenseMap+sort) when multiple decls are returned.
> > > WDYT?
> > Just to clarify the original suggestion.
> >
> > Maybe we can sort the `(location, is_explicit)` pairs instead of the
> > `(decl, is_explicit)` pairs?
> > Sorting based on pointer equality (see `L.D < R.D`) provides
> > non-deterministic results and we can have fully deterministic order on
> > locations.
> >
> > DeclarationAndMacrosFinder can return the results in arbitrary orders and
> > the client code would be responsible for getting locations and finally
> > sorting them.
> > WDYT?
> I think we'd better sort them in `DeclarationAndMacrosFinder` -- because we
> have a few clients in `XRefs.cpp` using this class, and it seems that they
> don't have their specific needs for sorting, having them sorting results
> seems unnecessary.
That LG, as long as we have a deterministic order, we should be fine
================
Comment at: clangd/XRefs.cpp:141
+ return false;
+ // Use the first child is good enough for most cases -- normally the
+ // expression returned by handleDeclOccurence contains exactly one
----------------
Do we know which cases break? Just wondering is there are more reliable ways to
handle those cases?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50438
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
