[PATCH] D50438: [clangd] Sort GoToDefinition results.

[Ilya Biryukov via Phabricator via cfe-commits]

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