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

Swh fix peek bytes #98

Merged
merged 2 commits into from Mar 9, 2018

Conversation

@pedantix
Copy link
Member

pedantix commented Mar 8, 2018

So I have learned a lot since I started this two days ago, this update should reflect that. I am not a pointer arithmetic pro again, yet, this PR should reflect a better grasp on reader indices and ByteBuffers.

pedantix and others added some commits Mar 8, 2018

Merge pull request #1 from vapor/nio
Synciring with vapor/Nio
Shaun Hubbard
extension ByteBuffer {
public func peekString(count: Int, skipping: Int = 0, encoding: String.Encoding) -> String? {
guard readableBytes >= count + skipping else { return nil }
guard let bytes = getBytes(at: skipping, length: count) else { return nil }
guard let bytes = getBytes(at: readerIndex + skipping, length: count) else { return nil }

This comment has been minimized.

@tanner0101

tanner0101 Mar 9, 2018

Member

D'oh. I should have caught this one. It also confused me at first that readFoo doesn't need the readerIndex but getFoo does... seems like NIO is missing a method in between that: gets at current index, but doesn't move the current index forward.

This comment has been minimized.

@tanner0101

This comment has been minimized.

@Lukasa

Lukasa Mar 9, 2018

I can see that, but I'm not 100% sure I agree there. It's definitely a bit of a question, but normally this isn't an issue because the read methods literally don't take an index at all: there's no read(offsetBy:length:) method. That said, I'd be open to being convinced that we do need them.

@tanner0101
Copy link
Member

tanner0101 left a comment

thanks :)

@tanner0101 tanner0101 added this to the 3.0.0-rc.2 milestone Mar 9, 2018

@tanner0101 tanner0101 self-assigned this Mar 9, 2018

@tanner0101 tanner0101 merged commit 90d00a8 into vapor:nio Mar 9, 2018

1 check passed

ci/circleci: linux Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment