Skip to content

Conversation

pavgust
Copy link
Contributor

@pavgust pavgust commented Aug 25, 2019

No description provided.

@pavgust pavgust requested a review from a team as a code owner August 25, 2019 14:59
@pavgust pavgust added this to the 1.22 milestone Aug 25, 2019
@jbj jbj added C++ Priority PR that should be reviewed and merged as a matter of priority. labels Aug 26, 2019
@jbj jbj merged commit 27b6ed3 into github:master Aug 26, 2019
@geoffw0
Copy link
Contributor

geoffw0 commented Aug 28, 2019

I'd like to make two minor observations:

(1) I'm not sure whether I've tested enough for this result to be significant but there appears to be a small but not negligible performance cost to this change on most snapshots (though I'm sure it does help greatly in pathological cases).

(2) This change is affecting at least one result in about half of the snapshots I tested, which is more than I expected. If it's supposed to affect only the most pathological cases, the threshold (10) could be much too low. On the other hand, if we think these results are confusing anyway, why isn't the number 2?

These things said, I'm quite happy with this change being in the release as-is.

@jbj
Copy link
Contributor

jbj commented Aug 30, 2019

(1) The lgtm.com dist was upgraded to deacc23 today, and that includes this PR. That means we can go explore lgtm.com to see if jump-to-definition seems to be working.

I picked this file arbitrarily and browsed through it. As far as I can tell, everything's in order. The only code without jump-to-def is macro arguments and code that's #ifdef'ed out.

(2) I agree that this PR was a temporary fix. Ideally, I see no reason the query should ever return more than one definition for a given use. That just means we store unnecessary data that'll be thrown away when the file is viewed because the viewer will pick arbitrarily among all the candidate definitions. It would be better to make this arbitrary choice in QL because (a) we know it'll be deterministic and (b) we can apply criteria such as preferring a definition in the current file over a definition in other files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Priority PR that should be reviewed and merged as a matter of priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants