Skip to content

Sort DeclAttributeAndRanges Deterministically #26099

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 12, 2019

Conversation

gmittert
Copy link
Contributor

@gmittert gmittert commented Jul 11, 2019

Comparing the start and end of ranges doesn't work in all cases since
both a < b and b < a can be true if a = b. This behavior was causing an
assertion fail in the Windows standard library which while sorting,
asserts that if a < b, then !(b < a) in debug mode.

Since ranges don't overlap otherwise, comparing just the starts of the
ranges will properly sort them.

@gmittert gmittert requested a review from akyrtzi July 11, 2019 22:26
@gmittert
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@akyrtzi akyrtzi left a comment

Choose a reason for hiding this comment

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

Good catch! Just a couple of nitpicks, otherwise LGTM!

});
std::sort(
SortedRanges.begin(), SortedRanges.end(),
[&](DeclAttributeAndRange LHS, DeclAttributeAndRange RHS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment that the comparison is like this because ranges may overlap along with a FIXME to investigate why there are overlapped ranges in the first place ?

Copy link
Contributor Author

@gmittert gmittert Jul 12, 2019

Choose a reason for hiding this comment

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

I believe it's not that there are overlapping ranges, but rather when comparing something against itself. So... a very specific instance of overlapping ranges.

Given that the ranges shouldn't overlap otherwise, would it be fair to just compare the start of each range rather than both the start and end?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so a simpler return SM.isBeforeInBuffer(LHS.second.Start, RHS.second.Start); would work as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I changed it to. I didn't realize before that the ranges shouldn't overlap otherwise, but that makes sense give that they're source ranges of attributes.

[&](DeclAttributeAndRange LHS, DeclAttributeAndRange RHS) {
return SM.isBeforeInBuffer(LHS.second.Start, RHS.second.Start)
? true
: SM.isBeforeInBuffer(RHS.second.Start, LHS.second.Start)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could simply use

: LHS.second.Start != RHS.second.Start

here.

@gmittert gmittert force-pushed the ATrueOrdering branch 2 times, most recently from b7c6b08 to f6dae49 Compare July 12, 2019 17:06
@gmittert
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - eaf1208de50c851a09d45717d828f5d7857ceefa

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - eaf1208de50c851a09d45717d828f5d7857ceefa

Comparing the start and end of ranges doesn't work in all cases since
both a < b and b < a can be true if a = b. This behavior was causing an
assertion fail in the Windows standard library which while sorting,
asserts that if `a < b`, then  `!(b < a)` in debug mode.

Since ranges don't overlap otherwise, comparing just the starts of the
ranges will properly sort them.
@gmittert
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f6dae49e4efc5a4e4bdfdfd761b21d06966a1ea8

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - f6dae49e4efc5a4e4bdfdfd761b21d06966a1ea8

@gmittert gmittert merged commit 7c78a7a into swiftlang:master Jul 12, 2019
@gmittert gmittert deleted the ATrueOrdering branch July 12, 2019 19:52
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.

3 participants