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

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

Closed
Fuuzetsu opened this issue Dec 7, 2023 · 4 comments · Fixed by #268
Closed

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

Fuuzetsu opened this issue Dec 7, 2023 · 4 comments · Fixed by #268

Comments

@Fuuzetsu
Copy link
Contributor

Fuuzetsu commented Dec 7, 2023

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
Copy link
Contributor Author

Fuuzetsu commented Dec 7, 2023

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
Copy link
Collaborator

frol commented Dec 7, 2023

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

borsh-rs/borsh/src/lib.rs

Lines 139 to 140 in 2b1f6c9

#[cfg(feature = "rc")]
pub use alloc::{rc, sync};

@frol frol closed this as completed Dec 7, 2023
@frol
Copy link
Collaborator

frol commented Dec 7, 2023

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

@frol frol reopened this Dec 7, 2023
@dj8yfo
Copy link
Collaborator

dj8yfo commented Dec 7, 2023

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

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 a pull request may close this issue.

3 participants