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

FromByteStream quite limited because only ByteRead functions can be used #14

Open
tp-m opened this issue Nov 23, 2023 · 5 comments
Open

Comments

@tp-m
Copy link

tp-m commented Nov 23, 2023

Apologies if this is a bit all over the place, but just wanted to quickly drop off a note on some issues I ran into whilst trying to use FromByteStream.

The following things I sometimes need when parsing certain types of data (from a slice of bytes):

  • remaining length
  • current position
  • read data to the end (not a problem if we can get the remaining length of course)

When using ByteReader directly that's no problem, since we can get access to underlying reader.

However, inside a FromByteStream implementation we only have the ByteRead trait to play with, which is very limited.

I wonder whether it would make sense to define ByteRead as pub trait ByteRead: Read?

In addition it would also be nice to be able to peek without consuming the data, but I guess that's independent.

cc @sdroege

@tuffy
Copy link
Owner

tuffy commented Nov 27, 2023

The trouble with ByteRead being a subtrait of Read is the name collision with the read() method, so that doesn't quite work.

But I've pushed out a new 1.9 version with reader_ref() and writer_ref() methods in the ByteRead and ByteWrite traits to get boxed references to the underlying reader/writer which you can downcast to concrete types, if necessary. Feel free to give it a try and let me know if you find them helpful.

Having a peekable reader that also implements reader methods would be a good idea, but I'll have to think of the best way to implement it.

@sdroege
Copy link
Contributor

sdroege commented Nov 27, 2023

Those don't have to be boxed. You can just return a &mut dyn Read etc there and not have one heap allocation per call :)

@sdroege
Copy link
Contributor

sdroege commented Nov 27, 2023

I'd suggest yanking 1.9.0 fast and making this change (without Box) as 1.9.1. Technically it would be a breaking change but nobody should've depended on the Box version of the API yet.

@sdroege
Copy link
Contributor

sdroege commented Nov 27, 2023

See #15

@tp-m
Copy link
Author

tp-m commented Nov 27, 2023

Many thanks @tuffy !

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

No branches or pull requests

3 participants