Skip to content
This repository was archived by the owner on Aug 15, 2025. It is now read-only.

Add impl Encode for [T], where T: Encode#542

Merged
VictorKoenders merged 3 commits intobincode-org:trunkfrom
cronokirby:trunk
Jun 5, 2022
Merged

Add impl Encode for [T], where T: Encode#542
VictorKoenders merged 3 commits intobincode-org:trunkfrom
cronokirby:trunk

Conversation

@cronokirby
Copy link
Contributor

Since Encode takes a reference, this allows us to encode &[T] directly
using this implementation. The encoding scheme is the same as for
Vec.

This also makes the implementation for &[u8] superfluous, since we get
an implementation for [u8] by virtue of u8 implementing encode. This
also gives us free implementations for &[u16], &[u32], etc. which is
quite useful. Nonetheless, we keep the implementation for &[u8] around,
because the implementation can directly write a large number of bytes,
which can be more efficient than the generic implementation.

And uh, apologies if you would have preferred I raise an issue first, but this felt like a small enough change to warrant a direct PR.

I actually ran into a situation where this was convenient. My workaround to not having this was to take &Vec<T> as an argument to my function, and this seemed highly unidiomatic. I was also surprised not to see this when digging around in the docs the other day.

@VictorKoenders
Copy link
Contributor

I think this will conflict with #526 and I want to get that PR merged in first, so this PR will have to stay open for a while.

Thank you for your contribution regardless!

@VictorKoenders
Copy link
Contributor

#526 is now merged.

Looks like this PR is still valid. You can remove the implementation on Box<[T]> because I think the implementation on [T] and Box<T> is strictly better.

Since Encode takes a reference, this allows us to encode &[T] directly
using this implementation. The encoding scheme is the same as for
Vec<T>.

This also makes the implementation for &[u8] superfluous, since we get
an implementation for [u8] by virtue of u8 implementing encode. This
also gives us free implementations for &[u16], &[u32], etc. which is
quite useful. Nonetheless, we keep the implementation for &[u8] around,
because the implementation can directly write a large number of bytes,
which can be more efficient than the generic implementation.
Since we've implemented Encode for [T], this makes the implementation
for Box<[T]> redundant (since we have a blanket implementation for
Box<T>), and ditto for &[T], which this change replaces by combining the
implementations for [T] and &T.
@cronokirby
Copy link
Contributor Author

Nice! I've removed redundant implementations for Box<T> as well as for &[T] (since it's made redundant since we now have both [T] and &T, and thus &[T]), the behavior for the latter is the same as before, so this should have no effect on end users.

@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #542 (5edb104) into trunk (dcda3a8) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            trunk     #542      +/-   ##
==========================================
- Coverage   54.52%   54.47%   -0.06%     
==========================================
  Files          49       49              
  Lines        4422     4417       -5     
==========================================
- Hits         2411     2406       -5     
  Misses       2011     2011              
Impacted Files Coverage Δ
src/features/impl_alloc.rs 65.34% <ø> (-0.96%) ⬇️
src/enc/impls.rs 89.17% <100.00%> (ø)
src/lib.rs 14.65% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcda3a8...5edb104. Read the comment docs.

@VictorKoenders VictorKoenders merged commit e8ccb0a into bincode-org:trunk Jun 5, 2022
@VictorKoenders
Copy link
Contributor

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants