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

Added get_mut_unchecked #232

Closed
wants to merge 16 commits into from
Closed

Added get_mut_unchecked #232

wants to merge 16 commits into from

Conversation

dynos01
Copy link

@dynos01 dynos01 commented May 29, 2023

I added get_mut_unchecked() as it might help someone.

Also, there are two things I'm considering but currently unsure about how to correctly implement:

  • Add wait_mut(). However mutable reference across threads requires Mutex, but waiting with Mutex acquired will block other threads from initializing the value. Is there a case that this will make sense?
  • Add get_mut_unchecked() in imp_cs.rs. This seems to require modification of the upstream crate critical_section, so I skipped it for now.

@dynos01
Copy link
Author

dynos01 commented May 29, 2023

For point 2, this does compile, but will it work as expected? (I don't have experience with crate critical-section, and the definitions of borrow and borrow_mut here look different)

pub(crate) unsafe fn get_mut_unchecked(&mut self) -> &mut T {
    debug_assert!(self.is_initialized());
    // SAFETY: The caller ensures that the value is initialized and access synchronized.
    crate::unwrap_unchecked(self.value.borrow_mut().get_mut().get_mut())
}

src/lib.rs Outdated
/// the contents are acquired by (synchronized to) this thread.
#[inline]
#[cfg(not(feature = "critical_section"))]
pub unsafe fn get_mut_unchecked(&self) -> &mut T {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the concrete use-case for this function? Given & -> &mut signature, this seems more or less impossible to use correctly.

Copy link
Author

Choose a reason for hiding this comment

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

My initial purpose was to implement wait_mut, but I soon realized that wait_mut seemed meaningless. But yes, it should be &mut -> &mut here. Since there are get_unchecked and get_mut, will a get_mut_unchecked look reasonable?

Copy link
Owner

Choose a reason for hiding this comment

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

I think that’s already available as get_mut().unwrap_unchecked()

Copy link
Author

Choose a reason for hiding this comment

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

Then what about a shortcut that's self-explanatory? I've added a new commit to fix the function signature, but if you believe this is not necessary, feel free to close this PR. Thanks.

@dynos01 dynos01 closed this Jun 5, 2023
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

4 participants