Skip to content

Switch BitstreamReader Entrypoints to Use ByteString #189

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
Feb 9, 2021

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Feb 8, 2021

ByteString is a more appropriate vocabulary type than Data to use for
these entrypoints. TSCBasic traffics in ByteString values, and its
representation as an array means that there are reasonable indexing
invariants we can rely on with respect to subsequences.

This should also enable some clean up in the swift-driver.

ByteString is a more appropriate vocabulary type than Data to use for
these entrypoints. TSCBasic traffics in ByteString values, and its
representation as an array means that there are reasonable indexing
invariants we can rely on with respect to subsequences.

This should also enable some clean up in the swift-driver.
@CodaFi
Copy link
Contributor Author

CodaFi commented Feb 8, 2021

@swift-ci test

@CodaFi CodaFi changed the title [NFC] Switch BitstreamReader Entrypoints to Use ByteString Switch BitstreamReader Entrypoints to Use ByteString Feb 9, 2021
Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

LGTM. Only concern would be performance — I don't know how optimized Data is internally compared with ByteString. But being able to return slices sounds as if it would be a performance win.

@CodaFi CodaFi merged commit 207f9c6 into swiftlang:main Feb 9, 2021
@CodaFi CodaFi deleted the reality-bytes branch February 9, 2021 23:58
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.

2 participants