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.

Sign up for GitHub

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

Document target_has_atomic #1171

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5316050
Document target_has_atomic
Mark-Simulacrum Feb 16, 2022
1ae3acc
Use relative link for core::sync::atomic
ehuss Apr 21, 2022
2325d95
Remove unstable API mention
Mark-Simulacrum Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/conditional-compilation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,28 @@ Example values:
* `"pc"`
* `"unknown"`

### `target_has_atomic`

Key-value option set for each bit width that the target supports
atomic loads, stores, and compare-and-swap operations.

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 •
edited
Loading

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.


[`core::sync::atomic`]: ../core/sync/atomic/index.html

Possible values:

* `"8"`
* `"16"`
* `"32"`
* `"64"`
* `"128"`
* `"ptr"`

### `test`

Enabled when compiling the test harness. Done with `rustc` by using the
Expand Down