-
Notifications
You must be signed in to change notification settings - Fork 36
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
Replace 'event' with 'tag' in explainer #161
Conversation
It was suggested to change the term 'event' to 'tag' to reference the section name and the entries within the section. The suggested reasons were they can be used something other than events in future, and the term event can be confusing with other concepts on the web. Closes WebAssembly#159.
In general, an event handler allows one to process an event generated by a block | ||
of code. Events suspend the current execution and look for a corresponding event |
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.
"event" should be replaced with "exception" wherever it is referring to a runtime exception, "exception tag" whenever it is referring to the static exception type, and just "tag" whenever it is referring to potential future contents of the tag section.
Similarly, it would be good to replace instances of "exception" with "exception tag" when they are referring to the static exception declaration.
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.
Thanks for the suggestion; I think the distinction makes the text a lot clearer. Fixed many parts of the existing text.
A new `tag` section is introduced. If included, it must appear immediately after | ||
the memory section. |
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.
It might be worth adding the caveat that custom sections can appear in between these two sections. Alternatively, we could say that the tag section is ordered between the memory section and the global section.
Edit: actually, we already say that below, so maybe this sentence isn't necessary?
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.
Thanks; removed.
| count | `varuint32` | count of the number of event to follow | | ||
| type | `event_type*` | The definitions of the event types | | ||
| count | `varuint32` | count of the number of tags to follow | | ||
| type | `tag_type*` | The definitions of the tag types | |
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.
Is tag_type
defined anywhere?
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.
While creating this PR, I found multiple occurrences that we had missed when we had replaced 'exception' with 'event' before, and I tried to replace 'event' with 'tag' in this PR, so I missed those remaining 'exception's.. It was written as exception_type
. I changed it to tag_type
now.
Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>
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 good, mostly nits.
|
||
| Field | Type | Description | | ||
|-------|------|-------------| | ||
| `attribute` | `varuint32` | The attribute of an exception. | | ||
| `attribute` | `varuint32` | The attribute of a tag. | |
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.
Obviously a separate issue, but I'm just noticing this while I'm here: I'd conservatively make this a uint8
for the time being, since we do not know yet how exactly this is gonna be used in the future, and a zero uint8
is still forward compatible with varuint32
. That's what we did in other places anyway.
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 can do that as a followup PR. But why uint8
better than varuint32
here?
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.
It's both more conservative and faster to decode. No reason to bother with allowing multibyte representations of 0 before we even know that we'll need them.
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
We recently decided to change 'event' to 'tag', and to 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161
We recently decided to change 'event' to 'tag', and 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161 Reviewed By: tlively Differential Revision: https://reviews.llvm.org/D104423
This changes the encoding of the `attribute` field, which is preserved for future use, from `varuint32` to `uint8`. `uint8` is simpler to encode/decode and this attribute is not likely to need `varuint32` range even in future. Suggested in WebAssembly#161 (comment).
We recently decided to change 'event' to 'tag', and to 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161
We recently decided to change 'event' to 'tag', and 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161
We recently decided to change 'event' to 'tag', and 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161
This changes the encoding of the `attribute` field, which is preserved for future use, from `varuint32` to `uint8`. `uint8` is simpler to encode/decode and this attribute is not likely to need `varuint32` range even in future. Suggested in #161 (comment).
We recently decided to change 'event' to 'tag', and 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161
The exception handling proposal spec was recently changed to rename the event section to the tag section, in order to use a clearer name that conflicts with fewer things in the web context: WebAssembly/exception-handling#161
The exception handling proposal spec was recently changed to rename the event section to the tag section, in order to use a clearer name that conflicts with fewer things in the web context: WebAssembly/exception-handling#161
#291) * Update wabt test submodule to latest * Update wasm-encoder linking tests for latest wabt A recent PR to wabt changed the printing slightly for these test cases: WebAssembly/wabt#1669 * Rename "events" to "tags" for exception handling The exception handling proposal spec was recently changed to rename the event section to the tag section, in order to use a clearer name that conflicts with fewer things in the web context: WebAssembly/exception-handling#161
We recently decided to change 'event' to 'tag', and 'event section' to 'tag section', out of the rationale that the section contains a generalized tag that references a type, which may be used for something other than exceptions, and the name 'event' can be confusing in the web context. See - WebAssembly/exception-handling#159 (comment) - WebAssembly/exception-handling#161 Reviewed By: tlively Differential Revision: https://reviews.llvm.org/D104423
It was suggested to change the term 'event' to 'tag' to reference the
section name and the entries within the section. The suggested reasons
were they can be used something other than events in future, and the
term event can be confusing with other concepts on the web.
Closes #159.