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(buffer): new api from_{bytes, array, iter} #1254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jetjinser
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 3973

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 80.199%

Totals Coverage Status
Change from base Build 3970: 0.01%
Covered Lines: 4350
Relevant Lines: 5424

💛 - Coveralls

Comment on lines +122 to +125
pub fn T::from_iter(iter : Iter[Byte]) -> T {
let arr = iter.collect()
T::from_array(arr)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

for byte in iter {
  buf.write_byte(byte)
}

And let the T::from_array just use T::from_iter?

Copy link
Contributor Author

@jetjinser jetjinser Jan 15, 2025

Choose a reason for hiding this comment

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

Should let @buffer.new use iter.count() as size_hint? This causes iter to be iterated twice: once for counting and once for writing, not sure if this counting op is cheaper than possible buffer expansion.

I guess arr.length() is cheaper than iter.count(), so I thought from_array should keep it as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

iter.count means run the iter once and count each element, which is not what we want. But I'm not quite sure about collecting first and then use T::from_array though. Because when we are collecting, we also have the underlying expansion logic upon the array. Also we would have an extra copy iteration.

So I think it would be reasonable to have two separate implementations instead of one relying on another.

/// Create a buffer from a bytes.
pub fn T::from_bytes(bytes : Bytes) -> T {
let buf = T::new(size_hint=bytes.length())
buf.write_bytes(bytes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should inline write_bytes's logic here, otherwise we would have an uncessary grow_if_necessary check inside the write_bytes.

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

Successfully merging this pull request may close these issues.

3 participants