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

Document the conditional existence of alloc::sync and alloc::task. #98218

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Jun 18, 2022

alloc declares

#[cfg(target_has_atomic = "ptr")]
pub mod sync;

but there is no public documentation of this condition. This PR fixes that, so that users of alloc can understand how to make their code compile everywhere alloc does, if they are writing a library with impls for Arc.

The wording is copied from std::sync::atomic::AtomicPtr, with additional advice on how to #[cfg] for it.

I feel quite uncertain about whether the paragraph I added to Arc's documentation should actually be there, as it is a distraction for anyone using std. On the other hand, maybe more reminders that no_std exists would benefit the ecosystem.

Note: target_has_atomic is stabilized but not yet documented in the reference.

The wording is copied from `std::sync::atomic::AtomicPtr`, with
additional advice on how to `#[cfg]` for it.
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 18, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2022
@kpreid
Copy link
Contributor Author

kpreid commented Jun 18, 2022

@rustbot label +A-docs

@rustbot rustbot added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Jun 18, 2022
@euclio
Copy link
Contributor

euclio commented Jun 18, 2022

The (unstable) doc_cfg attribute may be useful here, but I don't think it supports target_has_atomic yet.

//!
//! **Note**: This module is only available on platforms that support atomic
//! loads and stores of pointers. This may be detected at compile time using
//! `#[cfg(target_has_atomic = "ptr")]`.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this might be true today, but in the future I could imagine that we'd have some parts of this module available even without atomics of pointer width.

It's already a bit weird that alloc::sync is not a superset of core::sync, it seems; that seems unusual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this might be true today, but in the future I could imagine that we'd have some parts of this module available even without atomics of pointer width.

The documentation could then be revised at the same time the cfg is removed.

It's already a bit weird that alloc::sync is not a superset of core::sync, it seems; that seems unusual.

That sounds like something worth fixing for consistency, but this change is forward-compatible with that — the note on the modules would just be removed, and the note on Arc kept. (alloc::task is in the same situation — and so technically the Wake trait should have a similar note.)

@Mark-Simulacrum
Copy link
Member

Going to reassign to @joshtriplett for T-libs-api review, since this is sort of a new documentation element. (And for thoughts on the re-export of core::sync::atomic from alloc::sync).

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@workingjubilee workingjubilee added the A-atomic Area: Atomics, barriers, and sync primitives label Aug 17, 2022
@joshtriplett
Copy link
Member

The comments seem like a reasonable addition, given the current state.

I also agree that these modules should be supersets of the modules in core, and when we go to change that (which probably needs an FCP), we can also change the documentation at that time.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit 5dcc418 has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2022
Document the conditional existence of `alloc::sync` and `alloc::task`.

`alloc` declares

```rust
#[cfg(target_has_atomic = "ptr")]
pub mod sync;
```

but there is no public documentation of this condition. This PR fixes that, so that users of `alloc` can understand how to make their code compile everywhere `alloc` does, if they are writing a library with impls for `Arc`.

The wording is copied from `std::sync::atomic::AtomicPtr`, with additional advice on how to `#[cfg]` for it.

I feel quite uncertain about whether the paragraph I added to `Arc`'s documentation should actually be there, as it is a distraction for anyone using `std`. On the other hand, maybe more reminders that no_std exists would benefit the ecosystem.

Note: `target_has_atomic` is [stabilized](rust-lang#32976) but [not yet documented in the reference](rust-lang/reference#1171).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2022
Document the conditional existence of `alloc::sync` and `alloc::task`.

`alloc` declares

```rust
#[cfg(target_has_atomic = "ptr")]
pub mod sync;
```

but there is no public documentation of this condition. This PR fixes that, so that users of `alloc` can understand how to make their code compile everywhere `alloc` does, if they are writing a library with impls for `Arc`.

The wording is copied from `std::sync::atomic::AtomicPtr`, with additional advice on how to `#[cfg]` for it.

I feel quite uncertain about whether the paragraph I added to `Arc`'s documentation should actually be there, as it is a distraction for anyone using `std`. On the other hand, maybe more reminders that no_std exists would benefit the ecosystem.

Note: `target_has_atomic` is [stabilized](rust-lang#32976) but [not yet documented in the reference](rust-lang/reference#1171).
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#98218 (Document the conditional existence of `alloc::sync` and `alloc::task`.)
 - rust-lang#99216 (docs: be less harsh in wording for Vec::from_raw_parts)
 - rust-lang#99460 (docs: Improve AsRef / AsMut docs on blanket impls)
 - rust-lang#100470 (Tweak `FpCategory` example order.)
 - rust-lang#101040 (Fix `#[derive(Default)]` on a generic `#[default]` enum adding unnecessary `Default` bounds)
 - rust-lang#101308 (introduce `{char, u8}::is_ascii_octdigit`)
 - rust-lang#102486 (Add diagnostic struct for const eval error in `rustc_middle`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 098e8b7 into rust-lang:master Oct 3, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 3, 2022
@kpreid kpreid deleted the nostdarc branch October 4, 2022 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants