This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Sort GoToDefinition results.
ClosedPublic

Authored by hokein on Aug 8 2018, 4:54 AM.

Details

Summary

GoToDefinition returns all declaration results (implicit/explicit) that are
in the same location, and the results are returned in arbitrary order.

Some LSP clients defaultly take the first result as the final result, which
might present a bad result (implicit decl) to users.

this patch ranks the result based on whether the declarations are
referenced explicitly/implicitly. We put explicit declarations first.

This also improve the "hover" (which just take the first result) feature
in some cases.

Event Timeline

hokein created this revision.Aug 8 2018, 4:54 AM
ilya-biryukov added inline comments.Aug 13 2018, 4:33 AM
clangd/XRefs.cpp
71

NIT: maybe call Occurence instead? As this is actually a Decl with some extra data, computed based on the expression it originated from. Occurence seems like a nice that for that kind of thing.

101

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?

132

Maybe simplify to Decls[D] |= IsExplicitReferenced?

139

NIT: use early exits by inverting condition

hokein updated this revision to Diff 160544.Aug 14 2018, 2:50 AM
hokein marked 3 inline comments as done.

Address review comments.

ilya-biryukov added inline comments.Aug 28 2018, 9:33 AM
clangd/XRefs.cpp
101

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?

298–299

Maybe use explicit type here? Using 'auto' is a bit confusing.

299

Maybe use explicit type here too or rename the field from 'D' to something more descriptive (e.g. Decl )?

Sorry for the delays with this review

hokein updated this revision to Diff 163311.Aug 30 2018, 5:43 AM
hokein marked 3 inline comments as done.

Address review comments, return deterministic results.

Sorry, I just realized I didn't reply the comments in the first review (they were in my draft).

clangd/XRefs.cpp
71

I'm not sure what's a good name here, but I think we'd better avoid using Occurrence, because this term is used by xrefs (and we will have xrefs implementation in this file).

101

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.

132

Good point!

299

use explicit type here (I avoided using Decl as a variable name, because Decl is a type name in clang already).

ilya-biryukov accepted this revision.Aug 30 2018, 8:18 AM

LG, thanks.
And a question of what are the things we can accidentally misdetect as explicit

clangd/XRefs.cpp
101

That LG, as long as we have a deterministic order, we should be fine

141

Do we know which cases break? Just wondering is there are more reliable ways to handle those cases?

This revision is now accepted and ready to land.Aug 30 2018, 8:18 AM

@hokein, would it be fine if I submit this change for you?
It would be nice to get this fix in before you get back from vacation.

@hokein, would it be fine if I submit this change for you?
It would be nice to get this fix in before you get back from vacation.

Thanks, and sorry for the delay here, I was planning to submit it after D50958 is submitted (probably today?)-- because it requires some rebasing changes.

Thanks, and sorry for the delay here, I was planning to submit it after D50958 is submitted (probably today?)-- because it requires some rebasing changes.

Thanks for the update and sorry for disturbing on your leave time. If you want, happy to rebase and land this after D50958 is submitted (just so that you can enjoy your leave without further disturbance ;-))

hokein updated this revision to Diff 164013.Sep 5 2018, 4:44 AM

Rebase to master.

hokein updated this revision to Diff 164014.Sep 5 2018, 4:47 AM

Fix code style.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.