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

feat: add byte counter #51

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add byte counter #51

wants to merge 3 commits into from

Conversation

stringhandler
Copy link
Contributor

Adds the byte counter that was previously in tari_core

@CjS77 CjS77 added the P-merge label Nov 7, 2022
src/byte_counter.rs Show resolved Hide resolved
}

#[cfg(test)]
mod test {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another useful test could be to do multiple calls to write, in order to assert that the stored total length updates properly (and doesn't, for example, reset by mistake or something).

use std::io;

#[derive(Debug, Clone, Default)]
pub struct ByteCounter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add brief struct- and module-level documentation to indicate the intent? In this case it's pretty obvious from inspection, but nice to have for things like IDEs that provide it.


use std::{io, io::Read};

pub struct LimitedBytesReader<R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add brief struct- and module-level documentation to indicate the intent? In this case it's pretty obvious from inspection, but nice to have for things like IDEs that provide it.

}
impl<R: Read> Read for LimitedBytesReader<R> {
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
let read = self.inner.read(buf)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will (attempt to) fill the buffer even in the case where too many bytes are read. Is this the desired behavior? I would have thought that if an error is raised, the caller might expect the buffer to remain untouched.

let mut reader = LimitedBytesReader::new(3, inner);
let mut buf = [0u8; 3];
let output = reader.read(&mut buf).unwrap();
assert_eq!(output, buf.len());
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually check that the buffer was filled (or filled with the expected data), which would be useful to assert.

// in case of buffer with length strictly bigger than reader byte_limit, the code should throw an error
let mut new_buf = [0u8; 4];
let output = reader.read(&mut new_buf);
assert!(output.is_err());
Copy link
Contributor

Choose a reason for hiding this comment

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

If the above comment about buffers and error handling is adopted, would be useful to assert that the buffer is unchanged.

Co-authored-by: Aaron Feickert <[email protected]>
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.

3 participants