-
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
Combine throw and create #43
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.
Cool, thanks! I have a couple of suggestions but they are all nitpicks on terminology.
proposals/Level-1.md
Outdated
[exception index space](#exception-index-space). This index is referred to as | ||
the `exception tag`. The `tagged exception type` is the corresponding | ||
[exception index space](#exception-index-space). This static index refers to the | ||
corresponding runtime index that uniquely identifies the exception type, and is |
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.
Suggestion: Maybe it's even clearer if you avoid calling the runtime thing an "index" in the first place. How about changing wording to use "tag" right away?
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.
Sound good. Changing.
proposals/Level-1.md
Outdated
the `exception tag`. The `tagged exception type` is the corresponding | ||
[exception index space](#exception-index-space). This static index refers to the | ||
corresponding runtime index that uniquely identifies the exception type, and is | ||
called the `exception tag`. The `tagged exception type` is the corresponding |
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.
Terminology nitpick: Technically what's defined in the exception section is not a type. What an exception definition defines is a fresh tag, which is a runtime value that has the ascribed exception type (and multiple tags can have the same type yet be different tags). This may be nitpicking, but the difference is that it clarifies that (1) multiple tags can have the same type, and (2) dispatch is on values not types (types have no runtime representation in Wasm).
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.
Ok. reworking this text.
proposals/Level-1.md
Outdated
1. The `except` instruction which creates a WebAssembly instance of the | ||
corresponding tagged exception type, and pushes a reference to it onto the | ||
stack. | ||
1. The `throw` instruction which creates a WebAssembly instance of the |
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.
Suggestion: I would say it creates an exception value with the corresponding 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.
Ok. Reworking the code to match.
proposals/Level-1.md
Outdated
@@ -114,8 +113,7 @@ Exception tags are used by: | |||
### The exception reference data type | |||
|
|||
Data types are extended to have a new `except_ref` type. The representation of | |||
an exception type is left to the host VM, but its size must be fixed. The actual | |||
number of bytes it takes to represent any `except_ref` is left to the host VM. | |||
an exception type is left to the host VM. |
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.
"exception type" -> "exception value"
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.
Done
proposals/Level-1.md
Outdated
The `throw` throws the exception on top of the stack. The exception is popped | ||
off the top of the stack before throwing. | ||
The `throw` instruction takes an exception index as an immediate argument. That | ||
index is used to identify the tagged exception type to throw. The values to |
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.
Suggestion: "tagged exception type" -> "exception tag"
"values to throw" -> "argument values"
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.
Rewritten to match changes.
proposals/Level-1.md
Outdated
The `throw` instruction takes an exception index as an immediate argument. That | ||
index is used to identify the tagged exception type to throw. The values to | ||
throw (corresponding to the signature) are popped from the top of the stack, and | ||
an exception is created. This includes attaching a stack track to the exception. |
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.
Nit: The stack trace isn't part of the core semantics and its presence may depend on the embedder.
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.
Removed.
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 modulo comments!
proposals/Level-1.md
Outdated
@@ -177,23 +188,24 @@ If the selected catch block does not throw an exception, it must yield the | |||
value(s) expected by the corresponding catching try block. This includes popping | |||
the caught exception. | |||
|
|||
Note that a caught exception can be rethrown using the `throw` instruction. | |||
Note that a caught exception value can be rethrown using the `throw` |
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.
Typo: rethrow
instruction?
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.
Done
proposals/Level-1.md
Outdated
|
||
| Field | Type | Description | | ||
|-------|------|-------------| | ||
| `count` | `varuint32` | The number of types in the signature | | ||
| `type` | `value_type*` | The type of each element in the signature | | ||
|
||
Note: An `except_type` is not actually a type, just an exception definition that |
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.
Well, the exception type in an exn def is indeed the type of the defined tag, so I believe this note isn't necessary (and probably misleading).
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.
Removed.
Combine the throw and exception create instructions into a single 'throw' instruction. Add back from the original proposal the 'rethrow' instruction that can rethrow a caught exception.
Also cleans up cases some confusion of exception tags and exception indices. That is, the index is the static value used in the module, while the tag is the corresponding exception identifier for that index.