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 target_has_atomic #1171

Merged
merged 3 commits into from
Oct 4, 2022
Merged

Conversation

Mark-Simulacrum
Copy link
Member

@ehuss
Copy link
Contributor

ehuss commented Feb 17, 2022

Thanks! Can you say what the implications are for a target to implement these? Is it mostly that the items in core::sync::atomic are available? Would it make sense to include a link to ../core/sync/atomic/index.html?

Can this also include a small mention that the support for smaller sizes may be implemented by the larger ones? (If that makes sense?)

@ehuss
Copy link
Contributor

ehuss commented Mar 27, 2022

Ping @Mark-Simulacrum Just wondering if you can respond to the questions above?

@Mark-Simulacrum
Copy link
Member Author

Yes, it's on my to-do list. Have been quite busy last couple weeks, hoping to get to it soon.

Verified

This commit was signed with the committer’s verified signature.
xkeshav Keshav Mohta
@Mark-Simulacrum
Copy link
Member Author

Sorry for the long delay here. I've pushed up a new version which tries to cover those topics -- and links to the documentation. Hopefully that helps.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry, I pushed a small change to use a relative link before I decided to comment. We use relative links instead of https://doc.rust-lang.org/ so that they work offline and so that the linkchecker can handle them.

Comment on lines 199 to 203
When this cfg is present, all of the [`core::sync::atomic`] APIs are available for
the relevant atomic width with the exception of `from_mut` methods (currently
unstable), which additionally require that the primitive integer and atomic have
the same minimum alignment on the given target. No user-visible, stable cfg is
exposed for that method at this time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to drop the mention of from_mut? We generally don't cover nightly features in the reference or things that might happen in the future, or delve into the particulars of how the standard library is implemented. We don't have the capacity to track them here, and it isn't relevant to someone working on stable. If target_has_atomic_equal_alignment or something like it is stabilized in the future, then it can be added here.

Copy link
Member Author

@Mark-Simulacrum Mark-Simulacrum Apr 21, 2022

Choose a reason for hiding this comment

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

Yeah, I have mixed feelings on this, probably should have commented on it myself. I think there's no nice way to describe what this gives you without mentioning from_mut - saying that all APIs are available would be misleading. Maybe the right thing is to drop it here and go add a note to the docs for from_mut though. It's still misleading but perhaps less so.

Does that seem like a reasonable approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I think a mention in the from_mut docs sounds good. I don't know what the status or history of target_has_atomic_equal_alignment is, though, so I'm not certain what the story is for stabilizing from_mut.

But maybe I'm misunderstanding, as I don't see how from_mut is important. I think it makes sense to say "it makes the corresponding APIs available". That is, if I want to write very portable code, I can write:

#[cfg(target_has_atomic = "64")]
{
    let x = std::sync::atomic::AtomicU64::new(123);
    // ...
}

Perhaps another way to word it would be:

The presence of this cfg is primarily used to check for the availability of the atomic types in [core::sync::atomic] with the corresponding bit width.

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting back to this now -- the suggested text/snippet to me implies that

let mut v: u64 = 3;
#[cfg(target_has_atomic = "64")]
{
    let x = std::sync::atomic::AtomicU64::from_mut(&mut v);
    // ...
}

would also be portable, but in practice that's not the case and there's no way (on stable) to conditionally test for that method's existence. And it's pretty easy to make a mistake here, since the cfg and from_mut are true on the common platforms (IIRC, only false on i686 or so?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since from_mut is still unstable, I still think it should not be mentioned in the reference. I wasn't trying to imply that every function and method would be available. Just that the primary use case for target_has_atomic is to detect the presence of the types (AtomicU8, AtomicU16, …).

from_mut isn't available on stable, and I would think that target_has_atomic_equal_alignment (or something similar) would be good to stabilize beforehand. And the requirement of target_has_atomic_equal_alignment should be made clear in the documentation of the from_mut documentation (and anything else that requires it). In theory, something like the portability lint would catch mistakes like your example.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Mark-Simulacrum Just checking in again on this. I was wondering if you would be ok with removing the discussion around from_mut? I think that means just removing everything in the paragraph starting with with the exception of…. We generally avoid mentioning any unstable bits in the reference. When from_mut and/or target_has_atomic_equal_alignment is stabilized in the future, that information can be added to the reference as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I completely forgot about this. Yeah, I'm OK removing that for now. I'll push a commit to that effect in a moment.

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).
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).
@Mark-Simulacrum
Copy link
Member Author

@ehuss pushed the adjustment to just drop the mention of the unstable API.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

@ehuss ehuss merged commit 3c55838 into rust-lang:master Oct 4, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
Update books

## nomicon

1 commits in f53bfa056929217870a5d2df1366d2e7ba35096d..9c73283775466d22208a0b28afcab44db4c0cc10
2022-09-05 07:19:02 -0700 to 2022-09-30 07:31:22 +0900
- Fix typo (rust-lang/nomicon#380)

## reference

9 commits in a7cdac33ca7356ad49d5c2b5e2c5010889b33eee..f6ed74f582bddcec73f753eafaab3749c4f7df61
2022-09-19 17:39:58 -0700 to 2022-10-08 02:43:26 -0700
- Typo 'a' -> 'an' (rust-lang/reference#1280)
- One line one sentence for expressions and statements main chapters (rust-lang/reference#1277)
- Document let else statements (rust-lang/reference#1156)
- Document `label_break_value` in the reference (rust-lang/reference#1263)
- Document target_has_atomic (rust-lang/reference#1171)
- update 'unsafe' (rust-lang/reference#1278)
- Update tokens.md (rust-lang/reference#1276)
- One sentence, one line Patterns chapter (rust-lang/reference#1275)
- Use semver-compliant example version (rust-lang/reference#1272)

## rust-by-example

9 commits in 767a6bd9727a596d7cfdbaeee475e65b2670ea3a..5e7b296d6c345addbd748f242aae28c42555c015
2022-09-14 09:17:18 -0300 to 2022-10-05 08:24:45 -0300
- Make it clear that rustdoc uses the commonmark spec (rust-lang/rust-by-example#1622)
- Update defaults.md (rust-lang/rust-by-example#1615)
- added "see also" for the @ binding sigil (rust-lang/rust-by-example#1612)
- add more precision to the effects of --bin flag (rust-lang/rust-by-example#1607)
- create bar project in cargo/dependencies example (rust-lang/rust-by-example#1606)
- use consistent wording about type annotation (rust-lang/rust-by-example#1603)
- cast.md improvements (rust-lang/rust-by-example#1599)
- Fix typo in macros.md (rust-lang/rust-by-example#1598)
- Corrected mistaken "The" instead of "There" (rust-lang/rust-by-example#1617)

## rustc-dev-guide

2 commits in 9a86c04..7518c34
2022-10-07 18:34:51 +0200 to 2022-10-08 12:29:47 +0200
- Update debugging.md
- Use llvm subdomain for compiler-explorer link

## embedded-book

1 commits in 4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954..c533348edd69f11a8f4225d633a05d7093fddbf3
2022-09-15 08:53:09 +0000 to 2022-10-10 10:16:49 +0000
- Fix a typo in registers.md  (rust-embedded/book#330)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
Update books

## nomicon

1 commits in f53bfa056929217870a5d2df1366d2e7ba35096d..9c73283775466d22208a0b28afcab44db4c0cc10
2022-09-05 07:19:02 -0700 to 2022-09-30 07:31:22 +0900
- Fix typo (rust-lang/nomicon#380)

## reference

9 commits in a7cdac33ca7356ad49d5c2b5e2c5010889b33eee..f6ed74f582bddcec73f753eafaab3749c4f7df61
2022-09-19 17:39:58 -0700 to 2022-10-08 02:43:26 -0700
- Typo 'a' -> 'an' (rust-lang/reference#1280)
- One line one sentence for expressions and statements main chapters (rust-lang/reference#1277)
- Document let else statements (rust-lang/reference#1156)
- Document `label_break_value` in the reference (rust-lang/reference#1263)
- Document target_has_atomic (rust-lang/reference#1171)
- update 'unsafe' (rust-lang/reference#1278)
- Update tokens.md (rust-lang/reference#1276)
- One sentence, one line Patterns chapter (rust-lang/reference#1275)
- Use semver-compliant example version (rust-lang/reference#1272)

## rust-by-example

9 commits in 767a6bd9727a596d7cfdbaeee475e65b2670ea3a..5e7b296d6c345addbd748f242aae28c42555c015
2022-09-14 09:17:18 -0300 to 2022-10-05 08:24:45 -0300
- Make it clear that rustdoc uses the commonmark spec (rust-lang/rust-by-example#1622)
- Update defaults.md (rust-lang/rust-by-example#1615)
- added "see also" for the @ binding sigil (rust-lang/rust-by-example#1612)
- add more precision to the effects of --bin flag (rust-lang/rust-by-example#1607)
- create bar project in cargo/dependencies example (rust-lang/rust-by-example#1606)
- use consistent wording about type annotation (rust-lang/rust-by-example#1603)
- cast.md improvements (rust-lang/rust-by-example#1599)
- Fix typo in macros.md (rust-lang/rust-by-example#1598)
- Corrected mistaken "The" instead of "There" (rust-lang/rust-by-example#1617)

## rustc-dev-guide

2 commits in 9a86c04..7518c34
2022-10-07 18:34:51 +0200 to 2022-10-08 12:29:47 +0200
- Update debugging.md
- Use llvm subdomain for compiler-explorer link

## embedded-book

1 commits in 4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954..c533348edd69f11a8f4225d633a05d7093fddbf3
2022-09-15 08:53:09 +0000 to 2022-10-10 10:16:49 +0000
- Fix a typo in registers.md  (rust-embedded/book#330)
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
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/rust#32976) but [not yet documented in the reference](rust-lang/reference#1171).
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.

None yet

2 participants