der: Simplify Reader trait: peek_8_bytes

[dishmaker]

Continuation from #1228

Reader trait contains too many methods.
All reader methods should return bytes for interoperability and ease of implementation.

Currently peek_header returns der::Header which is a library struct (cyclic dependency).
I think it should be read by der crate itself, not the user crate.

formats/der/src/reader.rs

Lines 26 to 30 in 086ecd7

	/// Peek forward in the input data, attempting to decode a [`Header`] from
	/// the data at the current position in the decoder.
	///
	/// Does not modify the decoder's state.
	fn peek_header(&self) -> Result<Header>;

I've simplified peek_header and peek_byte into one function.

/// Peek at most 8 bytes (3 byte tag + 5 length)
fn peek_bytes(&self) -> &[u8];

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.

pub struct PemReader<'i> {
    /// Inner PEM decoder wrapped in a BufReader.
    reader: RefCell<utils::BufReader<'i>>,

I my implementation there's no need for runtime borrow checks.
https://github.com/dishmaker/formats/blob/master/der/src/reader/pem.rs#L132

pub struct PemReader<'i> {
    /// Inner PEM decoder wrapped in a BufReader.
    reader: utils::BufReader<'i>,

As you can see, everything is simpler with peek_bytes.

Tests

PEM works
dishmaker@8a49da4

dishmaker@dm:~/formats$ cargo test -p der 
Running tests/pem.rs (target/debug/deps/pem)

running 3 tests
test to_pem ... ok
test from_pem ... ok
test from_pem_peek ... ok

[tarcieri]

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.

[dishmaker]

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 ?

[dishmaker]

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];