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

Implement RefUnwindSafe for Atomic #136

Merged
merged 1 commit into from
Mar 2, 2024
Merged

Implement RefUnwindSafe for Atomic #136

merged 1 commit into from
Mar 2, 2024

Conversation

sarsko
Copy link
Contributor

@sarsko sarsko commented Feb 27, 2024

Atomics should be RefUnwindSafe, and currently they are not. This PR makes them RefUnwindSafe.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@bkragl bkragl left a comment

Choose a reason for hiding this comment

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

Two things that can be done separately.

First, it seems like Mutex and RwLock explicitly implement UnwindSafe, although that shouldn't be necessary.

Second, I think it would be nice to add tests that assert the auto traits that we expect our sync primitives to implement. Something like https://github.com/indexmap-rs/indexmap/blob/184fe4bfcba20e21242c924220170b98be157d45/src/map/core.rs#L636-L642.

@sarsko
Copy link
Contributor Author

sarsko commented Mar 2, 2024

First, it seems like Mutex and RwLock explicitly implement UnwindSafe, although that shouldn't be necessary.

True, though doing it explicitly is a low-cost way to avoid an accidental footgun (if their internals are ever changed). It also makes their documentation match that of their std counterpart, through moving UnwindSafe up from the "Auto Trait Implementations" section.

Second, I think it would be nice to add tests that assert the auto traits that we expect our sync primitives to implement. Something like indexmap-rs/indexmap@184fe4b/src/map/core.rs#L636-L642.

Yes, agreed.

@bkragl bkragl merged commit 29c782f into main Mar 2, 2024
5 checks passed
@bkragl bkragl deleted the RefUnwindSafe branch March 2, 2024 14:19
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.

2 participants