wren-lang / wren

The Wren Programming Language. Wren is a small, fast, class-based concurrent scripting language.
http://wren.io
MIT License
6.93k stars 555 forks source link

Using `int` instead of `WrenErrorType` in `WrenErrorFn` #1166

Closed MAJigsaw77 closed 1 year ago

mhermier commented 1 year ago

What is the use case?

MAJigsaw77 commented 1 year ago

What is the use case?

The use case doesn't change.

mhermier commented 1 year ago

If there is no use case, I don't see the point of that change. Lowering an enum to an int without a reason is error prone

MAJigsaw77 commented 1 year ago

If there is no use case, I don't see the point of that change. Lowering an enum to an int without a reason is error prone

WrenErrorFn has issues with casting in another languages such like Haxe and this pr makes this alot easier for casting the function.

mhermier commented 1 year ago

Can you expand the problem more. To me, it looks like a binding generator/tooling problem, and is therefore not a valid reason to perform that change.

MAJigsaw77 commented 1 year ago

Can you expand the problem more. To me, it looks like a binding generator/tooling problem, and is therefore not a valid reason to perform that change.

It is a bindings ploblems but most of the languages that made the bindings of this couldn't reproduce this function correctly (Most of the people who made them just removed the function entirely), but using int instead of typedef enum solves the ploblem really easy.

CrazyInfin8 commented 1 year ago

I haven’t used Haxe in a while and haven’t really been successful in creating bindings for Haxe to interface with C libraries but could you create a shim WrenErrorFn that accepts the WrenErrorType, converts that type to a Haxe compatible enum or int, then passes on those values to the users callback function?

In my bindings for Go: WrenGo (and a couple others I have done), I’ve used a switch to get the WrenErrorType and create a Go style error with the relevant information. The user does not provide wren with their WrenErrorFn directly, but passes it to WrenGo which takes Wren’s error in C, bundles it up and into a Go struct type, then passes that type to the users callback so they don’t have to worry about type issues.

MAJigsaw77 commented 1 year ago

I haven’t used Haxe in a while and haven’t really been successful in creating bindings for Haxe to interface with C libraries but could you create a shim WrenErrorFn that accepts the WrenErrorType, converts that type to a Haxe compatible enum or int, then passes on those values to the users callback function?

In my bindings for Go: WrenGo (and a couple others I have done), I’ve used a switch to get the WrenErrorType and create a Go style error with the relevant information. The user does not provide wren with their WrenErrorFn directly, but passes it to WrenGo which takes Wren’s error in C, bundles it up and into a Go struct type, then passes that type to the users callback so they don’t have to worry about type issues.

Haxe Callable functions doesn't accept a enum abstract that uses a Int instead of typedef enum, but using int instead instead of that solves it, this is mostly a Haxe issue but this is the easiest way to fix it.

CrazyInfin8 commented 1 year ago

You can still probably create a shim function in C which can take the WrenErrorType, convert it to int (in C, not Haxe), then passes that on to the Haxe function.

Another option is you could create another C function type similar to WrenErrorFn, then in C you can cast that function to a WrenErrorFn since C is less fussy about type safety than Haxe. Just ensure that the functions are more or less identical (except for the WrenErrorType being an int instead) and also make sure everything is the same byte size. This way, Haxe could use that function type instead and C would think they are simply the same.

MAJigsaw77 commented 1 year ago

In my bindings: hxwren, I'm using a submodule, but there's a way to use cpp glue code and then to use it on a class but that isn't the best way to do it tbh.

MAJigsaw77 commented 1 year ago

This is the reason why I made this pr

CrazyInfin8 commented 1 year ago

As you said this is probably mostly a Haxe issue.

I don't think a PR like this on Wren is worth it since this merely hides the problem that Haxe has. Changing WrenErrorType to int would also introduce some breaking changes potentially damaging other bindings and projects that use Wren.

I doubt a PR like this would pass but for the time being you could just maintain a fork with the sole purpose of working better with Haxe (at least until Haxe supports enum parameters in callbacks). Some of my other bindings also use my own fork of wren. Some modifications include adding userdata to ForeignMethodFns (because C likes functions predeclared, something that languages with closures might not do), a way to stop a running VM (If it was processing an infinite loop for example), and a way to check how much bytes are allocated by that VM (Something I used for WASM).

MAJigsaw77 commented 1 year ago

I think i'll leave this open (if they would want to push this pr) but until then I'll use a fork to fix that little issue

MAJigsaw77 commented 1 year ago

Ok, after I tested the fork more, I can tell that it works the like a charm