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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 30 08:19:11 PDT 2018

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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list