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

expose std::sync::atomic #355

Closed
wants to merge 2 commits into from
Closed

expose std::sync::atomic #355

wants to merge 2 commits into from

Conversation

yoshuawuyts
Copy link
Contributor

This exposes async_std::sync::atomic. If we want to document the sync submodule similar to std, we will need to expose this. I feel it's also nice to continue to move towards parity with std.

Thanks!

Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
@ghost
Copy link

ghost commented Oct 16, 2019

What if we just did pub use std::sync::atomic;?

@yoshuawuyts
Copy link
Contributor Author

@stjepang I did that, and it also exposes all the deprecated items. Figured this may look a bit cleaner (:

@nbdd0121
Copy link
Contributor

What if an architecture has some atomics (e.g. AtomicU64 on 32-bit platforms) missing?

@taiki-e
Copy link
Contributor

taiki-e commented Oct 16, 2019

@nbdd0121 The current async-std cannot be compiled on those targets (see #141).

@ghost
Copy link

ghost commented Oct 17, 2019

I'm a bit on the fence here. My impression is that atomics are not very common in async code and are kind of low-level (memory orderings, how do they work?) so I don't know if they belong into async-std.

@nbdd0121
Copy link
Contributor

@nbdd0121 The current async-std cannot be compiled on those targets (see #141).

But I believe that this project intends to support those platforms, so we'd better fix that issue and not introducing new platform-compatibility issues.

@yoshuawuyts
Copy link
Contributor Author

@stjepang I tend to err on the side of "eh, let's add it" if it directly re-exports a part of std in an existing submodule. I wanted to port std's docs, which talk about Atomics a bunch so figured we might as well export them 😅

@taiki-e
Copy link
Contributor

taiki-e commented Oct 18, 2019

But I believe that this project intends to support those platforms, so we'd better fix that issue and not introducing new platform-compatibility issues.

FYI: #286 fixes that issue.

@taiki-e
Copy link
Contributor

taiki-e commented Oct 20, 2019

I personally prefer to only include things that are related to async code.

Also, alloc::sync does not reexport core::sync::atomic for similar reasons. (see rust-lang/rust#58175)

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Oct 28, 2019

I don't feel too strongly about this anymore. I'm fine to close this.

@yoshuawuyts yoshuawuyts deleted the expose-atomics branch October 28, 2019 11:13
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