Navigation Menu

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

Implement TryFrom<&[u8]> for OwnedBinary #272

Closed
wants to merge 2 commits into from

Conversation

scrogson
Copy link
Member

@scrogson scrogson commented Nov 4, 2019

No description provided.

@filmor
Copy link
Member

filmor commented Nov 5, 2019

  1. With this one there are now essentially 3 versions of the same code: OwnedBinary::from_unowned, OwnedBInary::realloc_or_copy and this one. Can you consolidate them to a single implementation of "filling OwnedBinary"?
  2. Wouldn't an implementation of TryFrom<Borrow<[u8]>> make more sense to catch also cases like Vec<u8>?

@evnu
Copy link
Member

evnu commented Nov 14, 2019

OwnedBinary::from_unowned, OwnedBInary::realloc_or_copy and this one.

It would be nice if those functions also used write_all() like the function proposed here. Refactoring sounds reasonable.

TryFrom<Borrow<[u8]>>

The Rust documentation on Borrow advises to use AsRef if only a reference to a related type is needed, maybe this would be better?

@scrogson
Copy link
Member Author

scrogson commented Dec 9, 2019

I tried to do this, but unfortunately it appears that the compile doesn't like this:

error[E0119]: conflicting implementations of trait `std::convert::TryFrom<_>` for type `types::binary::OwnedBinary`:
  --> rustler/src/types/binary.rs:17:1
   |
17 | / impl<T> TryFrom<T> for OwnedBinary
18 | | where
19 | |     T: AsRef<[u8]>,
20 | | {
...  |
31 | |     }
32 | | }
   | |_^
   |
   = note: conflicting implementation in crate `core`:
           - impl<T, U> std::convert::TryFrom<U> for T
             where U: std::convert::Into<T>;

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.

I believe that we'd need specialization for this to work?

@scrogson scrogson closed this Dec 11, 2019
@filmor filmor deleted the owned_binary_try_from_vec_u8 branch June 6, 2020 22:15
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.

None yet

3 participants