-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve method specificity algorithm #22162
Conversation
91b25aa
to
5a65533
Compare
Could we please get rid of all variable names with < 6 characters in this file? I hate to impose some sort of arbitrary limit, but I've been having trouble following along with the recent PRs to this file since I can't keep straight what these acronyms mean. The aggressive compression of variable names in this PR (env -> e, akind -> ak, for example) is only making it worse. (a little more whitespace, so the structure of the code is visible at a glance rather than requiring actual parsing of the code, would also be much appreciated 🙂) |
Can I keep |
I will also have trouble thinking of > 6 character names for |
yes, you can keep |
b5d4269
to
8b83fa7
Compare
Ok I tried to do a few style improvements. Most importantly I think the tuple specificity algorithm itself is now far simpler. |
Woot! Thanks for checking (always nice to add to the "this closes n issues" stats 😉). And of course I'm especially grateful for the fix; |
simplify tuple type specificity implementation fix specificity `A{B,B,C} < A{B,C,D}`
8b83fa7
to
e92eb58
Compare
Is this backportable? |
This code needed some weeding and strengthening. First, I had the existing code write out a trace of all non-subtype specificity results computed during the system image build (45MB, and took so long just to parse that it prompted me to do #22161). Then I added assertions to ensure that
morespecific(a,b) => !morespecific(b, a)
during the algorithm, and otherwise tried to clean up the logic as much as I could. I tested that against the trace, fixed the cases that needed fixing, and extracted interesting cases into a new specificity test suite.This makes many changes to the specificity relation, but mostly involving disjoint types, so it does not seem to affect any code behavior, at least in the test suite. However it is very hard to know whether this will break anything.