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

Excise the HasRaw*Handle traits #139

Merged
merged 5 commits into from
Sep 10, 2023
Merged

Excise the HasRaw*Handle traits #139

merged 5 commits into from
Sep 10, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Aug 8, 2023

After reading #135, I've realized that the HasRawDisplayHandle and HasRawWindowHandle traits have little value in a post-borrowed-handle world. Borrowed handles do everything better, and the raw handle can be extracted from the borrowed handle. Therefore it makes sense to remove these traits.

Closes #135.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

This looks perfect honestly.

I could potentially see how an implementer might not want to provide the strong guarantees about validity that HasWindowHandle, but HasRawWindowHandle already had those guarantees, they were just more implicit, so I think this will be fine!

src/borrowed.rs Outdated Show resolved Hide resolved
src/borrowed.rs Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Sep 2, 2023

Removing it outright might be a bit too harsh. I've opted to deprecate it for now.

@madsmtm
Copy link
Member

madsmtm commented Sep 2, 2023

Maybe you could add a blanket unsafe impl<T: ?Sized + HasWindowHandle> HasRawWindowHandle for T { ... }? That would mean less work for crates wanting to implement these traits.

@notgull
Copy link
Member Author

notgull commented Sep 2, 2023

Maybe you could add a blanket unsafe impl<T: ?Sized + HasWindowHandle> HasRawWindowHandle for T { ... }? That would mean less work for crates wanting to implement these traits.

Banger idea, I've implemented this

@Lokathor
Copy link
Contributor

Lokathor commented Sep 2, 2023

What would you say people who don't have borrowing lifetimes on their handles do

@madsmtm
Copy link
Member

madsmtm commented Sep 2, 2023

What would you say people who don't have borrowing lifetimes on their handles do

I'm not sure I understand the question, but what would an example of this be? Like, what would be a situation where you, as a "provider" type, would like to say "hey, I have this window handle, its lifetime is not tied to me (&self), but I don't know the lifetime of it either"?

@Lokathor
Copy link
Contributor

Lokathor commented Sep 2, 2023

So SDL gives you *mut SDL_Window when you make a window.

and because pointers are Copy the "lifetime" of that pointer, as the compiler sees it, is not anything sensible because there's all sorts of spots where you'll accidentally get a copy of the value instead of extending the life of a borrow on the pointer, or some other fiddly thing like that.

So there's certainly some span of time that's correct (until you call SDL_DestroyWindow), but Rust's borrow checker won't know what the correct time is.

@madsmtm
Copy link
Member

madsmtm commented Sep 2, 2023

Makes sense. In this case, don't implement Has[Raw]WindowHandle for the pointer, because it doesn't have a window handle for its full lifetime.

Instead, expose an unsafe method like this:

/// # Safety
/// The window must not be destroyed for the lifetime of the handle
pub unsafe fn get_handle<'a>(window: *mut SDL_Window) -> WindowHandle<'a> { ... }

Note that WindowHandle<'_> implements HasWindowHandle, so graphics crates that take T: HasWindowHandle will still work. Note also that the lifetime here is unbounded, so the user can choose 'static if that's needed by their graphics crate.

@Lokathor
Copy link
Contributor

Lokathor commented Sep 2, 2023

Alright, as long as we've thought about it and we have some answer, then we can proceed

After reading #135, I've realized that the HasRawDisplayHandle and
HasRawWindowHandle traits have little value in a post-borrowed-handle
world. Borrowed handles do everything better, and the raw handle can be
extracted from the borrowed handle. Therefore it makes sense to remove
these traits.

Closes #135.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Sep 10, 2023

It seems to me that everyone is fine with this, so I'll merge it.

@notgull notgull merged commit e3f68a7 into master Sep 10, 2023
4 checks passed
@notgull notgull deleted the notgull/remove-raw branch September 10, 2023 20:06
@notgull notgull mentioned this pull request Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Type-system relationship between HasRaw*Handle and Has*Handle traits
3 participants