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

Add IO::Delimited #2973

Merged
merged 2 commits into from
Jul 14, 2016
Merged

Add IO::Delimited #2973

merged 2 commits into from
Jul 14, 2016

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Jul 9, 2016

From docs:

An IO that wraps another IO, and only reads up to the beginning of a specified delimiter.

This is useful for exposing part of an underlying stream to a client.

Also adds some convenience methods to Slice that are (or were) utilised in implementing IO::Delimited.

The algorithm for IO::Delimitedis a little complex, but I believe it's readable with the docs and comments I've added. It's also reasonably performant: benchmarks. It seems to be able to do about 1gbps/core, which should be enough for most usages.

def read(slice : Bytes)
return 0 if @slice.size == 0
max_read_size = {slice.size, @slice.size}.min
# read_size = {1, max_read_size / 2}.max
Copy link
Member

Choose a reason for hiding this comment

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

Leftover comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

@@ -239,6 +239,68 @@ struct Slice(T)
pointer(count).copy_to(target, count)
end

# Copies the contents of this slice into *target*.
#
# Truncates if this slice doesn't fit into the destination slice.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think silently truncating is good. If there's not enough room it should raise.

Are you using this "feature" in your code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of the API is that it copies the size of the smaller slice, and returns the amount of bytes it copies. I think this API is sane, but i'm not sure, i'm willing to be convinced either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, i'm using it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From IRC:

 <RX14> i don't want it to throw
 <jhass> but given all the slice stuff raises on bounds related things
 <jhass> it probably should remain a callside responsibility for now
 <RX14> no it raises when you try and ask it to do something stupid explicitly
 <RX14> that doesn't set a precedent for copying to raise if the user hasn't truncated the slices right

Copy link
Contributor Author

@RX14 RX14 Jul 12, 2016

Choose a reason for hiding this comment

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

I fixed this: It raises when src will not fit into dest, but not when src does not completely fill dest.

@RX14 RX14 force-pushed the feature/delimited-io branch 2 times, most recently from 7d513c0 to f76b83e Compare July 12, 2016 18:07
@RX14
Copy link
Contributor Author

RX14 commented Jul 14, 2016

Just fixed the style violation, anything else needed before merging?

@asterite
Copy link
Member

@RX14 Looks good now.

One last thing: maybe delimiter instead of read_delimiter? I think it's better if we use shorter names.

@RX14
Copy link
Contributor Author

RX14 commented Jul 14, 2016

@asterite well i kinda left it like that in case we ever wanted to add a write version, and the read_size made it into IO::Sized so I would feel compelled to change that if I changed this.

@asterite
Copy link
Member

You mean, you'd write until you find a delimiter being written? Well, in any case read_delimiter maybe makes it more explicit than it's for reading, and one can choose not to use the named argument anyway...

@RX14
Copy link
Contributor Author

RX14 commented Jul 14, 2016

@asterite those were my thoughts exactly

@asterite asterite merged commit 33697ab into crystal-lang:master Jul 14, 2016
@asterite
Copy link
Member

@RX14 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants