Navigation Menu

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

ByteBuffer: improve and simplify get* implementations #896

Merged
merged 5 commits into from Mar 13, 2019

Conversation

weissi
Copy link
Member

@weissi weissi commented Mar 11, 2019

Motivation:

For historic reasons, the get* implementations checked the same indices
repeatedly and the checks were also repetitive.

Modifications:

unify and de-deplicate get* range checks

Result:

@weissi weissi added 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) and removed 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 labels Mar 11, 2019
@weissi weissi added this to the 2.0.0 milestone Mar 11, 2019
Sources/NIO/ByteBuffer-core.swift Outdated Show resolved Hide resolved
return nil
}

return ByteBufferView(buffer: self, range: index0 ..< index0+length)
return ByteBufferView(buffer: self, range: index ..< (index + length))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use the range-generation function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa ByteBufferView uses (and has to use as Collection) absolute ByteBuffer indices so here, we're not interested in range relative to readable bytes but range relative to the whole ByteBuffer

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, I see that.

This represents a difficulty with the requirement that ByteBufferViews only index readable bytes: specifically, what is readable can change after the ByteBufferView is created, without invalidating the ByteBufferView itself. I don't think this particularly matters much, as this will only happen if you're going outside the standard NIO usage patterns, but it's worth being aware of.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa no, ByteBufferView holds a (private) ByteBuffer which is a value, so the readable bytes can't change.

Motivation:

For historic reasons, the get* implementations checked the same indices
repeatedly and the checks were also repetitive.

Modifications:

unify and de-deplicate get* range checks

Result:

fixes apple#884
@weissi weissi merged commit 357b931 into apple:master Mar 13, 2019
@weissi weissi deleted the jw-884 branch March 13, 2019 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve sub-optimal get* implementations in ByteBuffer check ByteBuffer docs for wrong info about get*
2 participants