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

Use doc(cfg) to annotate feature-gated items #1019

Merged
merged 6 commits into from
Aug 27, 2020
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Aug 19, 2020

Starts implementation of #986. @newpavlov / @vks thoughts? I haven't extended this to other crates yet pending feedback.

Note that applying to impls e.g. like this doesn't appear to do anything.

 // Implement `CryptoRng` for boxed references to an `CryptoRng`.
 #[cfg(feature = "alloc")]
+#[cfg_attr(nightly, doc(cfg(feature = "alloc")))]
 impl<R: CryptoRng + ?Sized> CryptoRng for Box<R> {}

rand_core/Cargo.toml Outdated Show resolved Hide resolved
@dhardy
Copy link
Member Author

dhardy commented Aug 21, 2020

Do we want this for #[cfg(feature = "std")] / alloc too? Possibly not since this may be confusing (esp. alloc).

@newpavlov
Copy link
Member

newpavlov commented Aug 21, 2020

This feature does not mark trait implementations, so IIUC alloc is not needed (since in public API it's only used for impls over Box), but for std I think Error::new and Error::inner should be marked.

@dhardy
Copy link
Member Author

dhardy commented Aug 21, 2020

I did the same for rand.

There are several items available only given alloc, e.g. SliceRandom::choose_multiple. Yes, we probably should document these too — but do we assume users know that std implies alloc for rand crates, or do we use e.g. #[cfg_attr(doc_cfg, doc(cfg(any(feature = "alloc", feature = "std")))]?

@newpavlov
Copy link
Member

I think we can assume it, at least it's what we do in RustCrypto.

@dhardy
Copy link
Member Author

dhardy commented Aug 21, 2020

Done.

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.

3 participants