[StackSwitching] Add resume_throw_ref#8140
Conversation
| // A ResumeThrow, with an empty Name() for the tag. | ||
| return makeResumeThrow(pos, annotations, type, Name(), resumetable); |
There was a problem hiding this comment.
I would prefer to have a makeResumeThrowRef method on IRBuilder so the parser doesn't need to worry about the representational details.
There was a problem hiding this comment.
PTAL at the commit "add IRBbuilder::makeResumeThrowRef" which adds that, but which also ends up adding a bunch of duplicated code. Do you think it's worth it?
There was a problem hiding this comment.
I think it would be worth it if we pulled that duplicated code out into a helper function as well.
There was a problem hiding this comment.
In the last commit I added a helper struct (as there are two fields we need, so a single return from a function isn't natural). It does save the duplication but also feels slightly odd to me...
| auto numHandlers = getU32LEB(); | ||
| std::vector<Name> tags; | ||
| std::vector<std::optional<Index>> labels; | ||
| tags.reserve(numHandlers); | ||
| labels.reserve(numHandlers); | ||
| for (Index i = 0; i < numHandlers; ++i) { | ||
| uint8_t code = getInt8(); | ||
| if (code == BinaryConsts::OnLabel) { | ||
| tags.push_back(getTagName(getU32LEB())); | ||
| labels.push_back(std::optional<Index>{getU32LEB()}); | ||
| } else if (code == BinaryConsts::OnSwitch) { | ||
| tags.push_back(getTagName(getU32LEB())); | ||
| labels.push_back(std::nullopt); | ||
| } else { | ||
| return Err{"ON opcode expected"}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's pulled this shared code out into a separate function.
| shouldBeEqual(curr->operands.size(), | ||
| tag->params().size(), | ||
| curr, | ||
| "resume_throw operands must match the tag"); |
There was a problem hiding this comment.
It would be good to add a TODO about actually checking the operand types as well.
| } | ||
|
|
||
| void maybeThrowAfterResuming(std::shared_ptr<ContData>& currContinuation) { | ||
| if (auto* tag = currContinuation->exceptionTag) { |
There was a problem hiding this comment.
It would be good to add an assertion that either exceptionTag has a value or exception has a value, but not both. Or we could put them both in a variant for even more safety.
Co-authored-by: Thomas Lively <tlively123@gmail.com>
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
LGTM with that last refactor.
| std::vector<Name> tags; | ||
| std::vector<std::optional<Index>> labels; | ||
|
|
||
| ResumeThrowData(const std::vector<OnClauseInfo>& resumetable) { |
There was a problem hiding this comment.
It might feel more lightweight to just return a pair of vectors or modify the vectors via outparams.
There was a problem hiding this comment.
Hmm, outparams would require declaring them twice, and returning a pair seems less clear than named fields in a struct... I guess I'll keep it as is, but we can refactor later if we decide to.
2 participants
Implement it as a variation on the existing ResumeThrow class: if the
tag exists, it is a
resume_throwwith that tag; if the tag is null, it isresume_throw_ref.Remove an assert in
visitTryTablein the interpreter: the code actuallyworks, and no TODO was necessary there: resuming "just works", like
with CallRef etc. (
visitTrydoes have a problem, resuming from atry's catch block - but try_table is fine.) This allows the test to pass.