-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[CS] Solve all conjunctions in source order #67800
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
Conversation
Previously we would only base the start loc on the `SubExpr`, but that isn't set until CSApply. Change it to take both `SubExpr` and `Body`'s source range into account. Also tighten up the invariant that a TapExpr must be created with a non-null BraceStmt.
Previously we would only do source ordering for ClosureExprs, but other conjunctions need to have their source location taken into account too, in order to make sure we don't try and type-check e.g a TapExpr in a second closure before we type-check the first closure. Also while here, switch to `std::min_element` instead of sorting, and treat invalid source locations as incomparable. rdar://113326835
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci please smoke test compiler performance |
@swift-ci please test compiler performance |
Thanks! I'll take a look later today or tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It seems like a good time to generalize favoredOverConjunction to account for the expanded scope conjunctions now have…
@swift-ci please smoke test compiler performance |
@swift-ci please test compiler performance |
A little concerned about the regression of SourceLinesPerSecond (incorrectly listed as an improvement), seems like more than just noise... |
I know that MovieSwift is flaky and there are UPASSes there too, you can check individual project status to check where this is coming from. |
The two runs seem pretty consistent, there are some sporadic UPASSes, but they're the same across both runs:
|
Running a baseline on #58827 to see whether we have an inherent slowdown for the first run (e.g maybe we're loading modules or something that gets cached for the second run, I would assume we'd nuke the workspace between runs tho). And yeah I'll download the logs and start seeing if it's specific to a given project. |
Sounds good! |
Baseline test also showed a 1% regression in NumSourceLinesPerSecond, despite not having any changes. Testing locally, it seems this counter is just flakey and unreliable. |
Previously we would only do source ordering for ClosureExprs, but other conjunctions need to have their source location taken into account too, in order to make sure we don't try and type-check e.g a TapExpr in a second closure before we type-check the first closure.
Also while here, switch to
std::min_element
instead of sorting, and treat invalid source locations as incomparable.rdar://113326835