-
Notifications
You must be signed in to change notification settings - Fork 553
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
[0.4.0] Introduce WrenLoadModuleResult, fix unfreed strings from host. #778
Conversation
The original attempt at handling the returns from loadModuleFn wasn't ideal. 889cae5 Instead of making the host go via the VM allocation and need to understand it semantically, we can instead solve the problem of the unfreed return result directly. This also opens up the option of providing a length parameter or other information needed later (length is optional, and not used as of right now, but exists to show intent).
I generally like this The one thing I'd potentially add is a user-data |
@DethRaid That's not a bad idea yea, to pass your engine or allocator pointers or what not. Also as a general option for callbacks in C some user data often makes sense. Added it. |
Will I agree the change is needed/correct, wouldn't it be better for the
future to change the callback as a foreign method?
|
@mhermier not sure what you mean by as a foreign? This form is consistent with all the callbacks in the user facing API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect!
@.ruby I mean a wren foreign function. It has some interest as future
proofing, ease of integration...
|
I don't see how it is related @mhermier, the signature for a foreign is |
It is related in the sense that it can ease future of virtualizing a wren
VM in pure wren. Choosing any other signature is fine but will add an
unnecessary extra translation layer to call a wren method.
|
But using a foreign function signature will add unnecessary pain to implement the simple and common use-case this callback was introduced for, right? The hard thing is still possible, but the easy thing is easy. |
Situation can be complicated depending at what level you are considering the API. Currently module evaluation is done by a specific pair of opcodes and machinery in the VM. But these opcode are not ultimately necessary. IMPORT_MODULE can be replaced by a wren foreign/function call that returns a module (and considering some comments from #775 and some previous discussion it will probably need to be a concrete thing), and IMPORT_VARIABLE, by reading a module like a |
But why? What problem does that solve? What are you trying to do that can't be done with the current syntax? Yeah sure, there's lots of things that could be done, which may or may not be more elegant from some theoretical perspective, but in my experience making those kinds of changes for their own sake leads to more problems than it solves |
@DethRaid what syntax are you talking about ? the It goes beyond the scope of this pull request but basically, the problem is how to create a wren VM within wren? But it goes deeper than that. There are only 2 critical things a VM should do: self state management and foreign calling convention. Everything else could be implemented using one or the other of these 2 components. Adding cruft around these fundamentals can be easily done, but it has to be done in a controlled manner, else the end result will loose flexibility/less self hosting... . Module loading is one of these cruft, but it can be easily reduced to a single foreign call returning a module. The question is do we want to leave this cruft as a design choice or not? There is no really good or bad answer to this, but some choices like this might be harder to fix in the future. |
The original attempt at handling the returns from loadModuleFn wasn't ideal.
See 889cae5, where it will attempt to free strings returned from the host unconditionally, assuming the host will allocate via the VM. This isn't a good workflow, and there are many origins for source that are not to be freed (and shouldn't be duplicated into VM memory for no reason).
Instead of making the host go via the VM allocation and need to understand it semantically, we can instead solve the problem of the unfreed return result directly. This also opens up the option of providing a length parameter or other information needed later (length is optional, and not used as of right now, but exists to show intent).
By making the callback part of the struct, it makes the result more useful in situations where you may return a fixed string (that shouldn't get freed) in one code path, and another in the same function where it must. Each returned string then, gets to decide whether it needs the callback, which simplifies host implementation significantly. Otherwise, logic from
loadModuleFn
needs to be duplicated into the complete callback redundantly, which can be bad for many reasons. This places the reasoning on the right side of the line at the call site.The basic usage is to return a struct with the source, instead of the source directly:
But as seen, an
onComplete
callback may be added to be notified when the vm is done with your source.As an additional note, this means that an (uncapturing) lambda can be used in place as well,