Conversation
| assert(expression->is<TryTable>()); | ||
| assert(index < static_cast<TryTable*>(expression)->catchTags.size()); | ||
| static_cast<TryTable*>(expression)->catchTags[index] = | ||
| catchTag ? Name(catchTag) : Name(); |
There was a problem hiding this comment.
@aheejin do we need to call refinalize() here so that sentTypes is repopulated?
There was a problem hiding this comment.
We don't have a Module anyway, so we can't get the type information from the tags.
There was a problem hiding this comment.
Then should we make this API receive the sent type?
Otherwise sentTypes will contain stale data after calls here?
There was a problem hiding this comment.
Yeah true... @ospencer Can we make this take sentTypes?
b15fd5c to
b0d708b
Compare
|
Realized I had the ordering wrong everywhere; should be good now. |
| assert(expression->is<TryTable>()); | ||
| assert(index < static_cast<TryTable*>(expression)->catchTags.size()); | ||
| static_cast<TryTable*>(expression)->catchTags[index] = | ||
| catchTag ? Name(catchTag) : Name(); |
There was a problem hiding this comment.
We don't have a Module anyway, so we can't get the type information from the tags.
|
@ospencer I opened ospencer#1 to sync the latest changes from |
|
Thanks! I merged it in. |
chharvey
left a comment
chharvey
left a comment
There was a problem hiding this comment.
didn’t get a chance to review the JS tests before… just some minor cleanups needed but looks good overall
| assert(theTryTable instanceof binaryen.TryTable); | ||
| assert(theTryTable instanceof binaryen.Expression); | ||
| assert(theTryTable.body === body); | ||
| assert(theTryTable.getNumCatches() == 2); |
There was a problem hiding this comment.
JS code should prefer strict equality (===) unless there's a good reason to use loose (==). In these tests, you’re checking against numbers (on lines 3325, 3330, etc.), so I would advise using ===.
| assert(info.hasCatchAll === theTryTable.hasCatchAll()); | ||
|
|
||
| theTryTable.body = body = module.unreachable(); | ||
| assert(theTryTable.body === body); |
There was a problem hiding this comment.
the previous line assigns theTryTable.body = body so not sure why the assertion is needed
|
|
||
| var newExnref = module.ref.null(binaryen.HeapType.noexn); | ||
| theThrowRef.exnref = newExnref; | ||
| assert(theThrowRef.exnref === newExnref); |
There was a problem hiding this comment.
also here, assertion is not needed since you just assigned theThrowRef.exnref = newExnref in previous line
|
Sorry, I didn't realize you didn't have commit access this needed merging. Will merge this in once you address new comments. |
5 participants
Adding these because I really want to use them. Let me know if there was a reason why these weren't there (I figure no one needed it yet).