-
Notifications
You must be signed in to change notification settings - Fork 55
Rebase this proposal on top of Module Linking #122
Conversation
767e57b
to
43fc088
Compare
43fc088
to
0aff428
Compare
I just want to say that I read the full overview in this pull request and I love it. Until now interface types were not super clear to me, I understood the basic idea but not the instructions nor the architecture, in particular with non-string types. The current explanation is well explained and seems to cover most of the needs I had identified for such a feature. Examples are clear and relevant, so congratulations for such a wonderful job! For me the part that really made it click was the list lifting explanation and example, this is where I suddenly understood the whole laziness and uniqueness problem, because it relates to traditionnal iterator problems. I think that actually the possibility to use list for iterators is very interesting and could be mentionned more prominently rather than being just a side note. All in all it just sounds great and I can't wait playing with it! |
Rendered version for the lazy like me. |
Can this be an exported alias instead? |
@lukewagner Are we close to a phase 2 stage with this PR?! |
a51377b
to
d3ed6a7
Compare
In the last commit, I merged and rewrote the last two use cases into a single one that I think more clearly states the high-level use case of "maximizing reuse" and how the proposals helps achieve that. Credit to @fgmccabe for the suggestions on how this could be improved. |
506c728
to
337989d
Compare
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.
Not finished.
Pretty sure we need destructors on lowering. E.g., exporting a function that takes a list as an argument: need to release the local space allocated for the list argument.
) | ||
) | ||
``` | ||
Imports can also be adapted, although doing so requires a bit more plumbing due | ||
to the acyclic nature of instantiation: |
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.
I had to read this a couple of times before I fully grokked it.
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.
Anything you'd add to clarify? (Note also the paragraph right under the code snippet that also adds some explanation.)
(alias (memory $libc $memory)) ;; make $libc.$memory the default memory | ||
|
||
(adapter_func $growingLowerByte (param u8 i32 i32 i32) (result i32 i32 i32) | ||
(let (param u8) (result i32 i32 i32) |
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.
I thought we were not going to use let
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.
The only problem is local
s (let
- or function-level); the param
used here is fine b/c it's a nameless operand on the stack.
) | ||
(adapter_func $lowerArray (param (list s32)) (result i32 i32) | ||
list.has_count | ||
if (param (list s32) i32) |
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.
This needs explaining.
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.
Oops, yeah. I added a paragraph under this code explaining the high points, lmkwyt.
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.
LGTM
Thanks for the review! |
In the current explainer, the mechanism by which
(@interface func)
s and(@interface implement)
s link with the core module's imports and exports is rather magical and limited. This linking can be explained and generalized in terms of the concepts introduced by Module Linking. Doing so also makes it much clearer what exactly is imported and exported in the final adapted module: it's the same story for when one module nests another.This "rebase" also pulls in a ton of discussion and iteration with @fgmccabe and @eqrion about the proposal in general. In particular:
string
type is now just an abbreviation for(list char)
(withchar
being added as a new scalar interface type)Lastly, use cases and additional requirements are updated to reflect our updated understanding. Happy to get feedback on this new draft!