search

near / borsh-rs

Rust implementation of Binary Object Representation Serializer for Hashing
https://borsh.io/
Apache License 2.0
287 stars 62 forks source link

Should `std` feature imply `rc` feature and vice-versa? #266

Closed Fuuzetsu closed 7 months ago

Fuuzetsu commented 7 months ago

I'm not sure why these are separate. I think you can't enable rc without std and if you enable std then you also have Rc and Arc whether you like it or not.

Ideally rc would just get removed but enabling it as part of std seems like the second best thing.

Fuuzetsu commented 7 months ago

I guess serde also has rc feature, along with this comment:

# Opt into impls for Rc<T> and Arc<T>. Serializing and deserializing these types
# does not preserve identity and may result in multiple copies of the same data.
# Be sure that this is what you want before enabling this feature.
rc = []
frol commented 7 months ago

@Fuuzetsu rc is available from alloc crate, so std is not required to enable rc.

https://github.com/near/borsh-rs/blob/2b1f6c93f3087ab7c987b0654fae75a26bc67723/borsh/src/lib.rs#L139-L140

frol commented 7 months ago

Though, we may indeed enable rc feature for std. @dj8yfo What do you think?

dj8yfo commented 7 months ago

it appears that serde added this feature for the sake of the warning in commentary about implications of behaviour on deserialization, thus (probably) forcing user to read the commentary before enabling it.

A logical way to continue is to add such commentaries to Cargo.toml and around BorshSerialize/BorshDeserialize impl as well and to default doc publish

[package.metadata.docs.rs]
features = ["derive", "unstable__schema"]  # add "rc" here