Skip to content

[ConstraintSolver] Fix shrink to use correct primary expression as a candidate #10849

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

Merged
merged 1 commit into from
Jul 10, 2017

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jul 10, 2017

When trying to identify candidates for shrinking we are missing the case
when apply expression is a source of the assignment operator, which leads
to incorrect results in some situations, because shrink is going to miss
some required contextual information about assignment.

Resolves: rdar://problem/33190087

…a candidate

When trying to identify candidates for shrinking we are missing the case
when apply expression is a source of the assignment operator, which leads
to incorrect results in some situations, because shrink is going to miss
some required contextual information about assignment.

Resolves: rdar://problem/33190087
@xedin xedin requested review from rudkx and DougGregor July 10, 2017 19:29
@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2017

@swift-ci please smoke test

@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2017

@swift-ci please test source compatibility

@slavapestov
Copy link
Contributor

Test case?

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 change looks safe. As I understand it, the removal of ReturnAllDiscovered resolves the perf. issue and the isSrcOfPrimaryAssignment is a more narrow fix for the previously-fixed source compatibility issue.

@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2017

@slavapestov The test-case for this is already included, it's just a different way to fix the original problem...

@rudkx
Copy link
Contributor

rudkx commented Jul 10, 2017

@xedin Is it possible to reduce the performance regression to something reasonable that we can add to the test suite? IIRC the case here is a new operator with many concrete overloads and a long chain of uses of the operator. That should be possible to add as a test case.

You could use the new solver time threshold option that I added to make it an error if it takes too long to compile. Just pick a "too long" value that will still pass with a debug ASAN-enabled build.

Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

LGTM!

@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2017

@rudkx I'll try to do that!

@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2017

Compatibility suite failed with the same UPass as all of its previous builds, which is unrelated to this patch.

@xedin xedin merged commit f6beec6 into swiftlang:master Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants