Skip to content
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

fix: conform pagemap extension to sequence protocol #321

Merged
merged 6 commits into from
Jul 26, 2024

Conversation

dhayab
Copy link
Member

@dhayab dhayab commented Jul 23, 2024

Summary

This PR updates a PageMap extension so that it conforms more closely to the Sequence protocol, which seems more restrictive with Xcode 16.

This also updates the CI to run tests on Xcode 16 and fixes a number of failing jobs.

Result

  • project can be built on Xcode 16
  • ci workflow works again

Fixes #318

@dhayab dhayab changed the title wip2 fix: conform pagemap extension to sequence protocol Jul 24, 2024
@dhayab dhayab mentioned this pull request Jul 24, 2024
Closed
@dhayab dhayab marked this pull request as ready for review July 24, 2024 14:36
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

changes look reasonable and CI passes, so LGTM, but as I don't have enough experience I'll let @aallam do the actual approval

Comment on lines +86 to +87
let elements = (self.startIndex..<self.endIndex).map { self[$0] }
return IndexingIterator(_elements: elements)
Copy link
Member

@aallam aallam Jul 26, 2024

Choose a reason for hiding this comment

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

I believe this creates a copy of the list elements, which should work. If performance issues arise, it might be wise to consider an alternative approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I see. I believe we can commit to it and monitor for customer feedback as you mentioned. Thanks for your review 🙇 !

Copy link
Contributor

@abdelmagied94 abdelmagied94 Nov 2, 2024

Choose a reason for hiding this comment

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

@aallam I have created this PR to prevent page items copying with each iterator creation, improving performance.

@dhayab dhayab merged commit a921ecb into master Jul 26, 2024
9 checks passed
@dhayab dhayab deleted the fix/pagemap-sequence-protocol branch July 26, 2024 13: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.

Xcode 16 beta build error
4 participants