Skip to content

Simplify atomics using lazy - #759

Closed
tamird wants to merge 2 commits into
rust-random:masterfrom
tamird:cleanup-atomics
Closed

Simplify atomics using lazy#759
tamird wants to merge 2 commits into
rust-random:masterfrom
tamird:cleanup-atomics

Conversation

@tamird

Copy link
Copy Markdown
Contributor

No description provided.

@tamird
tamirdforce-pushed the cleanup-atomics branch 6 times, most recently from 09f7584 to 3a40345CompareNovember 24, 2025 22:34
Comment threadsrc/lib.rs
#[allow(dead_code, reason = "not used in all backends")]
mod error;
#[allow(dead_code, reason = "not used in all backends")]
mod lazy;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can't unconditionally include lazy. It will cause breakage on platforms without AtomicUsize.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Which platorms, please? Are they not included in CI?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, well it used to be the riscv32imc platforms (which do not have atomics). However, for some reason lazy.rs is building, even on a target like riscv32imc-unknown-none-elf which definitely doesn't have atomics. I'll investigate furthur.

Comment threadsrc/lib.rs
use core::mem::MaybeUninit;

mod backends;
#[allow(dead_code, reason = "not used in all backends")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this, we only want #[allow(dead_code)] on certain methods in the error module.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

There are only two pub (crate) items on the error module and prior to this change they are both marked allow(dead_code). Which methods do you not want this on?

Comment threadsrc/lib.rs Outdated
Comment threadsrc/backends/netbsd.rs Outdated
@tamird
tamirdforce-pushed the cleanup-atomics branch 7 times, most recently from 320758f to d22e6e5CompareNovember 24, 2025 23:47

@newpavlovnewpavlov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not convinced that we should "simplify" the NetBSD backend by reusing the lazy module. The current code is fairly straightforward and self-contained, I don't see significant issues with it.

This PR also introduces a lot of other IMO unnecessary changes out of scope for the purpose declared in the PR title.

@tamird

Copy link
Copy Markdown
ContributorAuthor

Sure, I can break it up into more PRs if you folks prefer. I was just looking for ways to make things a bit more neat. Anyway, I used a macro to introduce LazyPtr which I think works nicely now.

@tamird
tamird requested a review from josephlrNovember 25, 2025 00:11
@tamirdtamird changed the title Simplify atomics using LazyUsizeSimplify atomics using lazyNov 25, 2025
@briansmith

Copy link
Copy Markdown
Contributor

I am not convinced that we should "simplify" the NetBSD backend by reusing the lazy module. The current code is fairly straightforward and self-contained, I don't see significant issues with it.

At least, I don't see how this change makes things easier to understand. The additional abstraction with the macros makes things harder to understand.

This PR also introduces a lot of other IMO unnecessary changes out of scope for the purpose declared in the PR title.
Sure, I can break it up into more PRs if you folks prefer.

+1.

@tamird

Copy link
Copy Markdown
ContributorAuthor

I am not convinced that we should "simplify" the NetBSD backend by reusing the lazy module. The current code is fairly straightforward and self-contained, I don't see significant issues with it.

At least, I don't see how this change makes things easier to understand. The additional abstraction with the macros makes things harder to understand.

This PR also introduces a lot of other IMO unnecessary changes out of scope for the purpose declared in the PR title.
Sure, I can break it up into more PRs if you folks prefer.

+1.

I'll mark this as draft - said other changes were in #760 (now #763)

@tamird
tamird marked this pull request as draft December 5, 2025 17:31
@newpavlov

Copy link
Copy Markdown
Member

Closing per my comment above.

Sign up for freeto 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.

4 participants

@tamird@briansmith@newpavlov@josephlr