-
Notifications
You must be signed in to change notification settings - Fork 82
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
formalize component value definitions #336
Conversation
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.
Great start, this is looking good.
e848d3a
to
5000176
Compare
5be7da1
to
33475cd
Compare
33475cd
to
96ff1c5
Compare
6192a31
to
b104874
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.
Nice work! This looks good to me, but I think @sunfishcode has also thought a lot about how to express component-model values in a similar context where we are not inferring the types from the value, but rather checking that they match a given target type, so I'd appreciate his review too.
b104874
to
fcf455b
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.
Great, thanks a bunch for all the changes (again). Just a few nits:
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.
Should those core:u8
and core:s8
be core:byte
instead?
Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
Uninterpreted integers can be written as either signed or unsigned and therefore elliminate the need for distinction between the two rules Signed-off-by: Roman Volosatovs <[email protected]>
Signed-off-by: Roman Volosatovs <[email protected]>
6e96444
to
6c8e7b4
Compare
I think this is ready for the next review round |
Signed-off-by: Roman Volosatovs <[email protected]>
c8894cc
to
a847d47
Compare
Signed-off-by: Roman Volosatovs <[email protected]>
a847d47
to
043b923
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.
Awesome, thanks again! I think that addresses all the feedback I'm aware of. I'll merge at the end of the week if nothing else pops up.
Special case `s8` and `u8` to ensure that exactly 1 byte is used to represent values of those types #336 (comment) Signed-off-by: Roman Volosatovs <[email protected]>
#336 (comment) Signed-off-by: Roman Volosatovs <[email protected]>
#336 (comment) Signed-off-by: Roman Volosatovs <[email protected]>
I used
component-model/design/mvp/Binary.md
Lines 363 to 367 in 145752a
component-model/design/mvp/Binary.md
Line 265 in 145752a
Tried to stay as close as possible to core Wasm spec and the canonical ABI spec.
uN
bitmask - this does favor cases where flags with lowest indexes are set, however I feel like this is a reasonable approach to take at least for consistency with all other numerical valuesu32
-s, same argument applies as above. I do feel like for most use cases the discriminant should fit in a single byte0x00
is used to identifyresult::ok
0x01
forresult:error
option::none
is0x00
option::some
is0x01
valtype
is specified once in the top-level(value t:<valtype> v:<val(t)>)
, because, although type inference should be possible in most use cases by looking at the usages of the value index, values may potentially be unused by the component directly (for example, exported). I feel like it's a very little price to pay for simpler implementation and type checking.val
encoding reference implementation is available at https://github.com/rvolosatovs/wrpc/blob/4fd7488caa4cff19079f851cd7d0efda8f613a64/crates/transport/src/lib.rs#L2557-L3223Refs #15