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

Remove unsafe from Window::from_ref and make Window clonable. #1367

Closed
iddm opened this issue Jan 26, 2024 · 1 comment
Closed

Remove unsafe from Window::from_ref and make Window clonable. #1367

iddm opened this issue Jan 26, 2024 · 1 comment

Comments

@iddm
Copy link
Contributor

iddm commented Jan 26, 2024

I think this "unsafe" is useless here, and the method is actually safe:

    #[inline]
    /// Create a new `Window` without taking ownership of the `WindowContext`
    pub const unsafe fn from_ref(context: Rc<WindowContext>) -> Window {
        Window { context }
    }

Having it is unsafe:

  1. Is not useful.
  2. It may confuse people who don't read. Go deeper to read the code.
  3. It may lead to people seeing it preferring not to store the Rc<WindowContext> but to have a double indirection and reference counting: Rc<Window> which internally has Rc<WindowContext> for no reason but to avoid unsafe calls.

Another thing to improve is:

pub struct Window {
    context: Rc<WindowContext>
}

This struct can easily derive Clone due to having Rc inside; why doesn't it do so? Now, if we want to have the rich interface of Window, we must either keep it reference counted (Rc<Window>) or store the Rc<WindowContext> and create the Window object back from it when we need to use it: Window::from_ref(window_context). I think this experience is suboptimal and doesn't bring any benefit.

If it is agreed to change in favour, there is a PR: #1368

@iddm iddm changed the title Remove unsafe from Window::from_ref Remove unsafe from Window::from_ref and make Window clonable. Jan 26, 2024
iddm added a commit to iddm/rust-sdl2 that referenced this issue Jan 26, 2024
Makes it clonable and removes the "unsafe" from the `Window::from_ref`
method.

Addresses Rust-SDL2#1367
@iddm
Copy link
Contributor Author

iddm commented Jan 30, 2024

Closed due to the PR having been merged.

@iddm iddm closed this as completed Jan 30, 2024
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

No branches or pull requests

1 participant