Skip to content

Conversation

@zayass
Copy link
Contributor

@zayass zayass commented Feb 15, 2018

Fixes wrong IndexSet range merging. Currently, all range extension collapse this range with next.

Bug introduced here fa68db0#diff-e7f44555b2cc650b187ee9d2cb0e9132R596

Affects 4.0, 4.1 and master

@spevans
Copy link
Contributor

spevans commented Feb 15, 2018

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Feb 15, 2018

Do these tests pass for IndexSet on Darwin? The implementations are similar I think.

@zayass
Copy link
Contributor Author

zayass commented Feb 15, 2018

@parkera yes it passes on Darwin
See this diff fa68db0#diff-e7f44555b2cc650b187ee9d2cb0e9132R596

It looks like a typo/copypaste error

Overlapping detects as nextRange.location + nextRange.length >= nextRange.location which alway true

@parkera
Copy link
Contributor

parkera commented Feb 15, 2018

Great, thanks for checking.

@parkera parkera self-requested a review February 15, 2018 19:53
@zayass
Copy link
Contributor Author

zayass commented Feb 15, 2018

@parkera Is it possible to merge this to 4.1 as well?

Looks like build broken by swift tests and foundation not even started to build.

@parkera
Copy link
Contributor

parkera commented Feb 15, 2018

Yah, we can take this on the swift-4.1-branch. Let's try the build again and see if we can get it to pass first though.

@parkera
Copy link
Contributor

parkera commented Feb 15, 2018

@swift-ci please test

@zayass
Copy link
Contributor Author

zayass commented Feb 16, 2018

Same result. Looks like swift master unstable today. So I will open another pr to 4.1

@zayass
Copy link
Contributor Author

zayass commented Feb 16, 2018

Hi, @parkera do you know whats going on with Jenkins build?

BTW I checked this tests on older platforms and it also has broken in 3.0 and 3.1 but in little bit different way

@alblue
Copy link
Contributor

alblue commented Feb 20, 2018

@swift-ci please test

@zayass
Copy link
Contributor Author

zayass commented Feb 22, 2018

Hi @parkera is it ok to merge? Or I should do something more?

@parkera parkera merged commit f9afb90 into swiftlang:master Feb 22, 2018
@parkera
Copy link
Contributor

parkera commented Feb 22, 2018

LGTM, thanks.

@zayass
Copy link
Contributor Author

zayass commented Feb 22, 2018

@parkera Could you look at this also #1437

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