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

Make GlFns::load_from take FnMut instead of Fn #8

Open
lunabunn opened this issue Apr 10, 2021 · 4 comments
Open

Make GlFns::load_from take FnMut instead of Fn #8

lunabunn opened this issue Apr 10, 2021 · 4 comments

Comments

@lunabunn
Copy link

GlFns::load_from currently takes Fn for the *GetProcAddress closure. AFAICT this unnecessarily restricts what the caller can do in their proc address closures, and it should take FnMut instead?

@Lokathor
Copy link
Owner

image

@Lokathor
Copy link
Owner

That said, can you demonstrate an actual use case of how this would allow code that isn't currently allowed.

Because function lookups should be a read-only sort of affair. If a function lookup is, by itself, modifying program state, then that's a very bad sign.

@lunabunn
Copy link
Author

lunabunn commented Apr 12, 2021

That said, can you demonstrate an actual use case of how this would allow code that isn't currently allowed.

To be frank, I have no idea. However, do note that glow hand-edits the signature of load_with (in its phosphorus-generated gl46.rs) to use FnMut because that's what its API takes. The gl crate also takes an FnMut. So the folks working on those crates must have had some reason? (Although I'm inclined to believe they just did it because they could, not for an actual use case)

@Lokathor
Copy link
Owner

Well, I wrote the current version of that code actually. I think I was just matching an old version to avoid semver breaking because glow started with gl.

However, I think gl is just being overbroad.

It's not impossible to change this but i dont think I want to without good reason because these loaders should be fully deterministic. (And if you absolutely need to track calls for debugging or whatever you could still use refcell.)

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

2 participants