Skip to content

[CSRanking] Fix func declaration ranking with default'ed parameters #13683

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 4, 2018
Merged

[CSRanking] Fix func declaration ranking with default'ed parameters #13683

merged 1 commit into from
Jan 4, 2018

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jan 3, 2018

If default'ed parameters in one decl are intermixed with non-defaulted
ones, skip claiming parameters in other decl at the same position.

Resolves: rdar://problem/36226874.

@xedin xedin requested a review from jrose-apple January 3, 2018 02:05
@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

@jrose-apple you've added tests which are not ambiguous now, so wanted to get your option on these changes.

@xedin xedin requested review from DougGregor and rudkx January 3, 2018 02:15
DougGregor
DougGregor approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look like an improvement, but I have to say that I'm bothered by isDeclAsSpecializedAs having this logic at all. Can't we re-use the argument-matching logic from the solver?

@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018 •
edited
Loading

@DougGregor Do you mean matchFunctionParamTypes or something else?

@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

@DougGregor I've changed it to use matchCallArguments to determine bindings, I think this is what you meant, is it? (There are couple of tests broken by this, I'll fix them up tomorrow).

@jrose-apple
Copy link
Contributor

I must have been very nervous when adding the fix-it about not wanting to change ranking too much (see #4785). It looks like I was just being cautious and didn't have any specific fears at the time, so if everyone's good with this then I'm happy to take the improvement!

@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

Ok, it looks like if we are not matching labels it doesn't really make much sense to use matchCallArguments so that's probably way this loop is there now, I'm going to revert to using that.

@xedin
[CSRanking] Fix func declaration ranking with default'ed parameters
0bae11b 
If default'ed parameters in one decl are intermixed with non-defaulted
ones, skip claiming parameters in other decl at the same position.

Resolves: rdar://problem/36226874.
@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

@swift-ci please test

@swiftlang swiftlang deleted a comment from swift-ci Jan 3, 2018
@swiftlang swiftlang deleted a comment from swift-ci Jan 3, 2018
@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Jan 3, 2018

@swift-ci please test macOS platform

@xedin xedin merged commit 11f98b2 into swiftlang:master Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

@jrose-apple jrose-apple Awaiting requested review from jrose-apple

@rudkx rudkx Awaiting requested review from rudkx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xedin @jrose-apple @DougGregor