glfw: Force init before use of init dependent functions #92

2021-11-21T19:51:02Z

InKryption commented

2021-11-21 19:51:02 +00:00

(Migrated from github.com)

Fixes #61.

This is very rough, just about barely working, and doesn't look to pretty, but tests pass on my machine now, so may as well put it up here.
That said, in doing this, I've started to dig my teeth into the error handling, and I've realized that a lot of errors in GLFW are less like errors, and more like band-aids over C's weak type system; the vast majority of instances of 'InvalidEnum' and 'InvalidValue' can be handled "implicitly" by Zig's type system.
This may take a while though.

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

Fixes #61. This is very rough, just about barely working, and doesn't look to pretty, but tests pass on my machine now, so may as well put it up here. That said, in doing this, I've started to dig my teeth into the error handling, and I've realized that a lot of errors in GLFW are less like errors, and more like band-aids over C's weak type system; the vast majority of instances of 'InvalidEnum' and 'InvalidValue' can be handled "implicitly" by Zig's type system. This may take a while though. - [x] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

InKryption commented

2021-11-21 19:55:10 +00:00

(Migrated from github.com)

Note: this wasn't all done by hand. A lot of this was repetitive, so I made handy use of regex and Search-Replace in VSCode, so there's bound to be plenty of not-so-natural-looking pieces.

emidoots (Migrated from github.com) reviewed

2021-11-21 19:59:15 +00:00

glfw/src/internal_debug.zig

					
				@ -0,0 +1,11 @@

				const std = @import("std");

This won't work, GLFW returns platform errors even sometimes in dynamic situations. For example, on Linux if X server does not support specific extensions.

emidoots (Migrated from github.com) reviewed

2021-11-21 20:03:10 +00:00

glfw/src/Cursor.zig

Would you mind breaking out the change for error denormalization (Error -> error{ InvalidEnum, PlatformError }) into a separate PR?

Actually, before that, it might be worth discussing if this is a good idea or not. I have a few concerns with this:

GLFW can change the errors functions return at any moment, we'll have to reflect that and it'll have ABI-breaking consequences for us but not for GLFW.
GLFW internally is not always consistent with the error values it says it returns in its documentation, unfortunately. Some functions can return errors that are not documented.

Would you mind breaking out the change for error denormalization (`Error` -> `error{ InvalidEnum, PlatformError }`) into a separate PR? Actually, before that, it might be worth discussing if this is a good idea or not. I have a few concerns with this: 1. GLFW can change the errors functions return at any moment, we'll have to reflect that and it'll have ABI-breaking consequences for us but not for GLFW. 2. GLFW internally is not always consistent with the error values it says it returns in its documentation, unfortunately. Some functions can return errors that are not documented.

emidoots commented

2021-11-21 20:05:07 +00:00

(Migrated from github.com)

I've realized that a lot of errors in GLFW are less like errors, and more like band-aids over C's weak type system; the vast majority of instances of 'InvalidEnum' and 'InvalidValue' can be handled "implicitly" by Zig's type system.

Totally agree with this, it's worth doing.

On the broader topic of GLFW error handling, hopefully you've also seen this: https://github.com/hexops/mach/tree/main/glfw#a-warning-about-error-handling

I'm not sure if there is a better way to represent GLFW errors, because they can be so "soft" or "hard" in different circumstances - but I wish there was, because as it stands error handling with GLFW is really easy to get subtly wrong

> I've realized that a lot of errors in GLFW are less like errors, and more like band-aids over C's weak type system; the vast majority of instances of 'InvalidEnum' and 'InvalidValue' can be handled "implicitly" by Zig's type system. Totally agree with this, it's worth doing. On the broader topic of GLFW error handling, hopefully you've also seen this: https://github.com/hexops/mach/tree/main/glfw#a-warning-about-error-handling I'm not sure if there is a better way to represent GLFW errors, because they can be so "soft" or "hard" in different circumstances - but I wish there was, because as it stands error handling with GLFW is really easy to get subtly wrong

👍 1

InKryption (Migrated from github.com) reviewed

2021-11-21 20:44:15 +00:00

glfw/src/Cursor.zig

Sorry, like I said, a lot of this was done mechanically to just get a working version up and passing. I'll work some regex magic when I get back on my laptop.

emidoots (Migrated from github.com) reviewed

2021-11-21 22:44:38 +00:00

glfw/src/Cursor.zig

BTW, if you like regexp find-and-replace for code, you might like this tool from my PL PhD friend: https://comby.dev - it's a bit nicer than regexp for doing code replacement operations

InKryption (Migrated from github.com) reviewed

2021-11-21 23:37:39 +00:00

glfw/src/Cursor.zig

That looks severely interesting, and quite useful. Thanks for the recommendation. I used my usual regex to revert the error denormalization, but I'll definitely be looking to try and use comby some time in the very near future.

emidoots (Migrated from github.com) reviewed

2021-11-22 05:03:15 +00:00

glfw/src/opengl.zig

					
				@ -160,6 +182,7 @@ pub const GLProc = fn () callconv(.C) void;

				///

I think so, and it's harmless to do so anyway

emidoots (Migrated from github.com) reviewed

2021-11-22 05:03:41 +00:00

glfw/src/vulkan.zig

					
				@ -103,6 +110,7 @@ pub const VKProc = fn () callconv(.C) void;

				///

				/// @thread_safety This function may be called from any thread.

same thought, yeah, we should

emidoots commented

2021-11-22 05:05:04 +00:00

(Migrated from github.com)

Really liking this 👍 only thing that's left here I think is the question around what to do with platform errors, we'll need to either sort that out or remove all the TODOs associated with that before merging I think

👍 1

InKryption (Migrated from github.com) reviewed

2021-11-22 12:55:46 +00:00

glfw/src/internal_debug.zig

					
				@ -0,0 +1,11 @@

				const std = @import("std");

Duly noted and done.

InKryption commented

2021-11-22 13:18:07 +00:00

(Migrated from github.com)

Yeah, after having actually looked through some of the source where 'GLFW_PLATFORM_ERROR' is set, it would be nigh-impossible to solve with this approach. Might still stew over a solution though.

There are still a fair few more TODO's which I'd like your input on.
In particular, what are your thoughts in regards to enum-ifying 'Joystick'? GLFW docs say they only support up to 16 joysticks (only 16 joystick IDs available), and the source re-inforces this by both asserting that any passed joystick ID be lteq to 16 (and gteq 0), conceptually mapping 'Joystick' to an exhaustive enum.

And also would like your thoughts on the functions that only have a potential for having the error 'GLFW_NOT_INITIALIZED' set that now should never return errors. We could remove the error altogether, but as you pointed out, if they decided to add any more errors to those functions, that would mean ABI incompatibility for us.
Though, now that I think about it, does GLFW have any guarantees about functions and the errors they can set? If not, then what I just said would technically apply to any function, meaning in the extreme, we'd have to normalize errors for every part of the API.

Yeah, after having actually looked through some of the source where 'GLFW_PLATFORM_ERROR' is set, it would be nigh-impossible to solve with this approach. Might still stew over a solution though. There are still a fair few more TODO's which I'd like your input on. In particular, what are your thoughts in regards to enum-ifying 'Joystick'? GLFW docs say they only support up to 16 joysticks (only 16 joystick IDs available), and the source re-inforces this by both asserting that any passed joystick ID be lteq to 16 (and gteq 0), conceptually mapping 'Joystick' to an exhaustive enum. And also would like your thoughts on the functions that only have a potential for having the error 'GLFW_NOT_INITIALIZED' set that now should never return errors. We could remove the error altogether, but as you pointed out, if they decided to add any more errors to those functions, that would mean ABI incompatibility for us. Though, now that I think about it, does GLFW have any guarantees about functions and the errors they can set? If not, then what I just said would technically apply to any function, meaning in the extreme, we'd have to normalize errors for every part of the API.

emidoots commented

2021-11-22 17:51:38 +00:00

(Migrated from github.com)

On the topic of platform errors, I should note why I am so hesitant about trying to remove those. Back in 2015 with the Go GLFW bindings I led an effort to eliminate the possibility of those being returned but we ultimately got clarification from the author of GLFW that even in e.g. system misconfiguration cases, platform errors could be returned https://github.com/glfw/glfw/issues/450

In particular, what are your thoughts in regards to enum-ifying 'Joystick'? GLFW docs say they only support up to 16 joysticks (only 16 joystick IDs available), and the source re-inforces this by both asserting that any passed joystick ID be lteq to 16 (and gteq 0), conceptually mapping 'Joystick' to an exhaustive enum.

Fully agree with this. I actually thought I had enumified these, but I guess not. One important note, the enum value should be c_int I think to keep interoperability with other libraries that expect to be able to receive a Joystick ID.

And also would like your thoughts on the functions that only have a potential for having the error 'GLFW_NOT_INITIALIZED' set that now should never return errors. We could remove the error altogether, but as you pointed out, if they decided to add any more errors to those functions, that would mean ABI incompatibility for us.

Though, now that I think about it, does GLFW have any guarantees about functions and the errors they can set? If not, then what I just said would technically apply to any function, meaning in the extreme, we'd have to normalize errors for every part of the API.

I think in cases where we have eliminated all documented errors the GLFW function could return, we should indeed remove the error return.

GLFW's guarantees around what errors a function may generate are somewhat aspirational, somewhat concrete. We faced the same issue with Go GLFW bindings back in the day, and you can see here that GLFW's author suggests the documentation today is generally correct https://github.com/glfw/glfw/issues/361#issuecomment-287236051

I think that there will definitely be cases where GLFW changes the errors a function may generate in a minor or patch release version, for example if using a new Wayland API to improve support for something they really have no choice. That said, I am content with us being generally aspirational here as well and trying to focus just on the documented errors and, when a breakage does change on the GLFW side, us also breaking ABI compatibility and either documenting such small changes can occur or releasing a new major version each time.

I'd rather have a nice API that sometimes require users update their ABI usage, than have an API that can return everything under the sun.

On the topic of platform errors, I should note why I am so hesitant about trying to remove those. Back in 2015 with the Go GLFW bindings [I led an effort to eliminate the possibility of those being returned](https://github.com/go-gl/glfw/issues/127#issuecomment-77964686) but we ultimately got clarification from the author of GLFW that even in e.g. system misconfiguration cases, platform errors could be returned https://github.com/glfw/glfw/issues/450 > In particular, what are your thoughts in regards to enum-ifying 'Joystick'? GLFW docs say they only support up to 16 joysticks (only 16 joystick IDs available), and the source re-inforces this by both asserting that any passed joystick ID be lteq to 16 (and gteq 0), conceptually mapping 'Joystick' to an exhaustive enum. Fully agree with this. I actually thought I had enumified these, but I guess not. One important note, the enum value should be c_int I think to keep interoperability with other libraries that expect to be able to receive a Joystick ID. > And also would like your thoughts on the functions that only have a potential for having the error 'GLFW_NOT_INITIALIZED' set that now should never return errors. We could remove the error altogether, but as you pointed out, if they decided to add any more errors to those functions, that would mean ABI incompatibility for us. > > Though, now that I think about it, does GLFW have any guarantees about functions and the errors they can set? If not, then what I just said would technically apply to any function, meaning in the extreme, we'd have to normalize errors for every part of the API. I think in cases where we have eliminated all documented errors the GLFW function could return, we should indeed remove the error return. GLFW's guarantees around what errors a function may generate are somewhat aspirational, somewhat concrete. We faced the same issue with Go GLFW bindings back in the day, and you can see here that GLFW's author suggests the documentation today is generally correct https://github.com/glfw/glfw/issues/361#issuecomment-287236051 I think that there will definitely be cases where GLFW changes the errors a function may generate in a minor or patch release version, for example if using a new Wayland API to improve support for something they really have no choice. That said, I am content with us being generally aspirational here as well and trying to focus just on the documented errors and, when a breakage does change on the GLFW side, us also breaking ABI compatibility and either documenting such small changes can occur or releasing a new major version each time. I'd rather have a nice API that sometimes require users update their ABI usage, than have an API that can return everything under the sun.

InKryption commented

2021-11-22 19:33:39 +00:00

(Migrated from github.com)

On the topic of platform errors, I should note why I am so hesitant about trying to remove those. Back in 2015 with the Go GLFW bindings I led an effort to eliminate the possibility of those being returned but we ultimately got clarification from the author of GLFW that even in e.g. system misconfiguration cases, platform errors could be returned glfw/glfw#450

That's unfortunate to hear, but good to know. Maybe farther in the future, mach could have patches over GLFW to fall more in line with Zig's error handling philosophy; but that would be a ways off.

Fully agree with this. I actually thought I had enumified these, but I guess not. [...]

I went ahead and did the enum-ification, though I opted to just make the member 'jid' into an enum, to avoid having to fix all the references to 'Joystick' as a struct file. Maybe that's its own smaller PR.

[...]
I'd rather have a nice API that sometimes require users update their ABI usage, than have an API that can return everything under the sun.

I share the sentiment wholeheartedly, though your comment concerning error-denormalization made me think ABI was a bigger concern than I had realized (not to say that it isn't a concern at all now). But taking that into account, I went ahead and removed the error union returns from functions that shouldn't return any errors. I may have missed some, but unsure if you want to block the PR on tracking those down (I'm happy to drudge around).

> On the topic of platform errors, I should note why I am so hesitant about trying to remove those. Back in 2015 with the Go GLFW bindings I led an effort to eliminate the possibility of those being returned but we ultimately got clarification from the author of GLFW that even in e.g. system misconfiguration cases, platform errors could be returned glfw/glfw#450 That's unfortunate to hear, but good to know. Maybe farther in the future, mach could have patches over GLFW to fall more in line with Zig's error handling philosophy; but that would be a ways off. > Fully agree with this. I actually thought I had enumified these, but I guess not. [...] I went ahead and did the enum-ification, though I opted to just make the member 'jid' into an enum, to avoid having to fix all the references to 'Joystick' as a struct file. Maybe that's its own smaller PR. > [...] I'd rather have a nice API that sometimes require users update their ABI usage, than have an API that can return everything under the sun. I share the sentiment wholeheartedly, though your comment concerning error-denormalization made me think ABI was a bigger concern than I had realized (not to say that it isn't a concern at all now). But taking that into account, I went ahead and removed the error union returns from functions that shouldn't return any errors. I may have missed some, but unsure if you want to block the PR on tracking those down (I'm happy to drudge around).

emidoots commented

2021-11-22 19:48:50 +00:00

(Migrated from github.com)

That's unfortunate to hear, but good to know. Maybe farther in the future, mach could have patches over GLFW to fall more in line with Zig's error handling philosophy; but that would be a ways off.

I think this dips into "rewrite GLFW" territory 😅 I'd refocus that effort on seeing if we could improve GLFW upstream, and steer clear of patching GLFW on our side as much as reasonably possible (UB is a clear exception here.)

your comment concerning error-denormalization made me think ABI was a bigger concern than I had realized

Sorry that was ambiguous.. When I wrote "might be worth discussing if this is a good idea or not" I really did mean discuss :D I just hadn't thought through the problem enough to think if that was a good idea or not.

Ideally, we can keep PRs on the smaller side in general - I'm super happy with more iterative development (even if that means littering TODOs throughout the code that we address in follow up PRs), primarily because that makes it easier to have these discussions.

e.g. ideally a PR titled this way focuses on one problem (force init before use of init dependent functions) and all other work ends up in other PRs so we can discuss merits of changes individually.

Anywho, this PR looks pretty great in its current state to me - if you want to mark it as ready for review I'm happy to merge now. If you want to, feel free to rebase and I'll merge all the commits - otherwise I'll squash merge to eliminate the merge commit.

> That's unfortunate to hear, but good to know. Maybe farther in the future, mach could have patches over GLFW to fall more in line with Zig's error handling philosophy; but that would be a ways off. I think this dips into "rewrite GLFW" territory 😅 I'd refocus that effort on seeing if we could improve GLFW upstream, and steer clear of patching GLFW on our side as much as reasonably possible (UB is a clear exception here.) > your comment concerning error-denormalization made me think ABI was a bigger concern than I had realized Sorry that was ambiguous.. When I wrote "might be worth discussing if this is a good idea or not" I really did mean discuss :D I just hadn't thought through the problem enough to think if that was a good idea or not. Ideally, we can keep PRs on the smaller side in general - I'm super happy with more iterative development (even if that means littering TODOs throughout the code that we address in follow up PRs), primarily because that makes it easier to have these discussions. e.g. ideally a PR titled this way focuses on one problem (force init before use of init dependent functions) and all other work ends up in other PRs so we can discuss merits of changes individually. Anywho, this PR looks pretty great in its current state to me - if you want to mark it as ready for review I'm happy to merge now. If you want to, feel free to rebase and I'll merge all the commits - otherwise I'll squash merge to eliminate the merge commit.

InKryption commented

2021-11-22 19:52:45 +00:00

(Migrated from github.com)

Squashing sounds good to me.

👍 1

emidoots (Migrated from github.com) approved these changes

2021-11-22 19:58:41 +00:00

emidoots (Migrated from github.com) left a comment

Seriously, thanks so much for the work you are doing :)

Sign in to join this conversation.

No reviewers

emidoots