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 'static requirement on run #3006

Merged
merged 3 commits into from
Aug 5, 2023

Conversation

kchibisov
Copy link
Member

There's no need to force the static on the users, given that internally some backends were not using static in the first place.

  • Tested on all platforms changed
  • [-] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

This would work for async-winit. In fact, it would work very well, since we could remove the 'static bound on the future in block_on.

However, it looks like this might be unsound on web to my uninformed eye,

src/platform_impl/web/event_loop/mod.rs Show resolved Hide resolved
@kchibisov
Copy link
Member Author

@daxpedda @madsmtm @rib

I guess we can't do that for a reason that some backend no-return basically. However, maybe we should just give up and say that users should use spawn/run on web/ios respectively?

@madsmtm
Copy link
Member

madsmtm commented Aug 5, 2023

I think the macOS backend is currently unsound if 'static is not set, since we may not be clearing the callback consistently.

Though that should be fixable.

Re if the backend doesn't ever return, then not having 'static is still fine.

@kchibisov
Copy link
Member Author

kchibisov commented Aug 5, 2023

@madsmtm the thing is that we return, but the API consumes the EventLoop so the user can't rerun things, so EventLoop itself is dropped and users can't create it again.

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

@madsmtm I was mostly curios about ios.

@daxpedda
Copy link
Member

daxpedda commented Aug 5, 2023

This should work fine for Web.
Rust actually just added support for the exception handling proposal, but only with no_std, which wasm-bindgen doesn't support. But when it actually hits run() on Web will be even weirder then before and actually breaks without 'static. I can add a check to make run() not available if exception handling is enabled.

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

I can fix that.

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.

I'm not sure I understand you, but I was trying to say that the change should be sound on iOS (even though it doesn't compile as-is).

@daxpedda
Copy link
Member

daxpedda commented Aug 5, 2023

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

I can fix that.

Required an unsafe conversion, but works.

@kchibisov
Copy link
Member Author

Required an unsafe conversion, but works.

Yeah, that's what I was afraid of, so I left it as is, unless a backend maintainer approves.

@kchibisov
Copy link
Member Author

@madsmtm you've replied about macOS initially though, not ios though. I guess your reply about ios was indirect though.

Does it matter for macOS how you cleanup, if you basically drop the EventLoop, like 'static shouldn't matter once you literally call drop on the entire loop in the end of run invocation?

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

For Web this works, but probably not useful and I would argue makes run() more and more problematic.

We can have another discussion about whether we should leave run() on Web or not in a different place and time.

@kchibisov
Copy link
Member Author

I guess I'll do the beta 2 with it like that, and then we could probably gather some feedback whether we'd really need it to keep that way.

kchibisov and others added 3 commits August 6, 2023 01:10
There's no need to force the static on the users, given that internally
some backends were not using static in the first place.
@kchibisov kchibisov merged commit 8100a6a into rust-windowing:master Aug 5, 2023
@kchibisov kchibisov deleted the run-no-static branch August 5, 2023 21:57
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.

4 participants