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_auto_cfg #1763

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 29, 2023

If we use #![cfg_attr(docsrs, feature(doc_auto_cfg))] instead of #![cfg_attr(docsrs, feature(doc_cfg))] we no longer need to manually mark types with #[cfg_attr(docsrs, doc(cfg(feature = "std")))].

Sweeeeeet.

Props to pezcore for the lesson :)

If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.

Sweeeeeet.
@Kixunil
Copy link
Collaborator

Kixunil commented Mar 29, 2023

I'm not really sure, if we do this we will need to check if any odd cfgs (like rust_v_x) get rendered correctly. IIRC there were some issues with this. If this renders correctly today then I guess I'm fine with this.

@tcharding
Copy link
Member Author

tcharding commented Mar 29, 2023

From my investigation just now it looks like this PR actually improves the situation.

Build docs on master

impl<'a> From<&'a Script> for Arc<Script>
Available on target_has_atomic="ptr" only.

Build docs on this branch

impl<'a> From<&'a Script> for Arc<Script>
Available on non-rust_v_1_60 or target_has_atomic="ptr only.

(Did not check other instances so there may be regressions elsewhere. I only checked this one and Bound (in script/borrowed.rs), and found that we forgot to mark the delegate_index! call with the correct attribute.)

@tcharding
Copy link
Member Author

If this renders correctly today then I guess I'm fine with this.

I'm surprised you are not more enthusiastic for this change, 100 lines of red in the diff coupled with less maintenance and never having to remember to add the additional attribute - seems like a big win to me.

@tcharding
Copy link
Member Author

Note to self, if this merges we need to change #1127 to remove the attribute from CONTRIBUTING.md.

@apoelstra
Copy link
Member

Nice! I also suspected that it would improve the situation somehow, because surely we couldn't have gotten it all correct doing things by hand ... am glad to be proven correct.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a189942

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 30, 2023

Cool, you've convinced me.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a189942

@apoelstra apoelstra merged commit 895db71 into rust-bitcoin:master Mar 30, 2023
@tcharding tcharding deleted the 02-29-doc-auto-cfg branch March 30, 2023 22:07
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