Make BorrowedBuf and BorrowedCursor generic over the data#149749
Make BorrowedBuf and BorrowedCursor generic over the data#149749joshtriplett wants to merge 1 commit intorust-lang:mainfrom
BorrowedBuf and BorrowedCursor generic over the data#149749Conversation
Replace uses of `u8` with a generic type. This will allow reusing `BorrowedBuf` elsewhere in the standard library, for purposes other than byte buffers.
rustbot
added
the
S-waiting-on-review
rustbot
added
the
T-libs
joshtriplett
added
T-libs-api
| pub struct BorrowedBuf<'data> { | ||
| /// | ||
| /// The type defaults to managing bytes, but can manage any type of data. | ||
| pub struct BorrowedBuf<'data, T = u8> { |
There was a problem hiding this comment.
I would prefer not having u8 as a default. It's not a huge deal to have users write it out and we don't really have a precedent for hiding such things.
There was a problem hiding this comment.
The main motivation for doing so, apart from convenience, was to localize the changes so everything depending on BorrowedBuf and BorrowedCursor didn't all have to change at once.
I do think, though, that u8 is going to be by far the most common case here, given the hopefully widespread use of read_buf.
There was a problem hiding this comment.
While I agree that u8 will be the most common case, I don't think this is a good justification for hiding the type. I don't see any harm in requiring users to explicitly indicate that they are taking a buffer of u8. Just like how read/write explicitly take [u8] slice.
There was a problem hiding this comment.
I think default type params can lead to frustrating and confusing bugs, particularly silent errors that work for upstream but break downstream. I have had several of these when refactoring data structures to use the unstable Allocator trait. A lint that demands all generic parameters be provided explicitly sounds too complex for the AST-based approach I believe clippy is limited to.
One particular concern I have with the u8 default is assuming that slice length in elements is always equivalent to the number of bytes, which could interact poorly with the unsafe set_init() method. I can very easily imagine the default leading to an upstream crate later deciding they want to support more types, and in the process failing to multiply by size_of::<T>(). I see alignment being potentially even trickier.
I do see #150129 potentially improving this, but I believe the concern remains for the BorrowedCursor struct.
| /// on the data in that buffer by transitivity). | ||
| #[derive(Debug)] | ||
| pub struct BorrowedCursor<'a> { | ||
| pub struct BorrowedCursor<'a, T = u8> { |
|
☔ The latest upstream changes (presumably #150106) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I think it'd be fine to land this PR for generic-izing the type, then we can land another PR right afterwards to remove the default and update the uses.
Code looks good to me, and I tried to scan for anything making unsaid assumptions about T (being Copy, Freeze, etc) and didn't find any that weren't already updated.
So r=me with conflicts fixed.
Aside: excited to try this out with generic types. Might be a great way to solve some of the Iterator::next_chunk or Vec::push_within_existing_capacity scenarios.
| } | ||
|
|
||
| impl Debug for BorrowedBuf<'_> { | ||
| impl<T> Debug for BorrowedBuf<'_, T> { |
There was a problem hiding this comment.
I was surprised to not see T: Debug, but I guess that's not a this-PR discussion.
scottmcm
added
S-waiting-on-author
6 participants
Replace uses of
u8with a generic type. This will allow reusingBorrowedBufelsewhere in the standard library, for purposes other thanbyte buffers.
As discussed in libs-api; cc @Amanieu.