Open dishmaker opened 8 months ago
Reader trait contains too many methods.
If you're suggesting it has too many required methods, that's reasonable. But the existing peek_*
methods should be retained as provided methods.
As for this:
/// Peek at most 8 bytes (3 byte tag + 5 length)
fn peek_bytes(&self) -> &[u8];
It's pretty unclear/counterintuitive from the name peek_bytes
what the semantics are going to be, especially someone encountering the function name in source code as opposed to reading documentation. I would personally expect a peek_bytes
function to take a number of bytes to peek, and if it didn't, it would be confusing as to how many it might return.
This is the reason why I didn't expose it in the first place: you're leaking semantics of how the peek buffer is implemented, and peek_bytes
doesn't tell you what they are.
A name that specifically identifies the function is returning the contents of the peek buffer, e.g. peek_buffer
, might be clearer, but there are magic numbers you are encoding into the function's contract.
there are magic numbers you are encoding into the function's contract.
I agree.
The only solution i see here is naming fn peek_up_to_8_bytes
However, for most use cases, it would never return more than 4 (1 byte tag + 3 byte length).
Naming things is hard... maybe peek_tag_length
?
there are magic numbers you are encoding into the function's contract.
I don't see any other way to make it clearer. Such function name may seem stupid, but if it works, then it's not stupid.
/// Peek at most 8 bytes (3 byte tag + 5 length)
fn peek_8_bytes(&self) -> &[u8];
Continuation from https://github.com/RustCrypto/formats/issues/1228
Reader
trait contains too many methods. All reader methods should return bytes for interoperability and ease of implementation.Currently
peek_header
returnsder::Header
which is a library struct (cyclic dependency). I think it should be read by der crate itself, not the user crate. https://github.com/RustCrypto/formats/blob/086ecd7d27aa3f6c5c97dc2a33adc7159008086b/der/src/reader.rs#L26-L30I've simplified
peek_header
andpeek_byte
into one function.Dependency and clone
Also,
peek_header
is a cyclic dependecy, because reading it requires another reader. PemReader accompishes it by.clone()
ing itself. That's a bad idea, because it enforced the usage of RefCell - to be clone'able.I my implementation there's no need for runtime borrow checks. https://github.com/dishmaker/formats/blob/master/der/src/reader/pem.rs#L132
As you can see, everything is simpler with
peek_bytes
.Tests
PEM works https://github.com/dishmaker/formats/commit/8a49da40fad44200e9a9b56b0d3a88e001077b89