Add support for optional catch binding syntax#5310
Conversation
|
Nice! Thanks, @zenparsing . Two things that would be good to confirm are that heap enumeration and F12 debugging can handle this syntax. There are heap enum tests in the full repo and tests in DebuggerCommon in the core repo that you can use as a model for testing F12 enumeration of locals. |
| try { | ||
| throw new Error(); | ||
| } catch { | ||
| throw new Err(); |
There was a problem hiding this comment.
is there a rethrow syntax as well?
There was a problem hiding this comment.
Taking a look at the proposal it doesn't look like it. Seems like having throw; would have been interesting (like C# does), but it seems like if you want to rethrow then you'll need the parameter binding.
There was a problem hiding this comment.
Personally I like that you still have to be explicit if you want to rethrow, I always found throw; in C++ to be kind of obtuse.
| ParseNodeCatch::ParseNodeCatch(OpCode nop, charcount_t ichMin, charcount_t ichLim) | ||
| : ParseNodeStmt(nop, ichMin, ichLim) | ||
| { | ||
| this->pnodeParam = nullptr; |
There was a problem hiding this comment.
hmmm interesting. Could you initialize this at initialization list of the constructor?
|
Thanks for mentioning that. Also I'd like few test to be added with new syntax which are under debugger and heap-enum. In reply to: 397101019 [](ancestors = 397101019) |
zenparsing
force-pushed
the
optional-catch-binding
branch
from
54695a0 to
2e20c88
Compare
|
Are we not putting this under a flag? |
|
Can you add couple more test cases with eval, var leaking in catch scope, with scope in catch block and function definitions inside catch block? |
|
@aneeshdk - we should have flag to on and off this feature. But I'd vote for having this enable by-default ON. |
|
@zenparsing are there any test262 tests for this feature and do we know how many pass/fail? |
zenparsing
force-pushed
the
optional-catch-binding
branch
from
2e20c88 to
0b2e0b0
Compare
zenparsing
force-pushed
the
optional-catch-binding
branch
from
0b2e0b0 to
efeab8f
Compare
|
@zenparsing @akroshg @aneeshdk What is left to get this landed? |
zenparsing
force-pushed
the
optional-catch-binding
branch
2 times, most recently
from
d2464c1 to
eca32ed
Compare
|
@kfarnung I've updated tests and rebased, but I have not yet implemented a flag for this feature. |
|
Do we still want an always-true flag for this? It's been unflagged in v8 for a while now. |
|
If the syntax is sure to stay in the spec, I think we can do without the flag. |
zenparsing
force-pushed
the
optional-catch-binding
branch
from
eca32ed to
133f30a
Compare
Merge pull request #5310 from zenparsing:optional-catch-binding Adds support for [optional catch binding syntax](https://tc39.github.io/proposal-optional-catch-binding), now at stage 4 and implemented in V8, SM, and JSC. This is my first PR; I'll appreciate any and all feedback! Resolves #5210
|
Is this patch never going to reach production because Edge is switching to a Chromium backend now? This SO question leaves devs with the impression that Edge should either support the syntax already, or is going to at some point in the future. Is that still true? |
9 participants
Adds support for optional catch binding syntax, now at stage 4 and implemented in V8, SM, and JSC.
This is my first PR; I'll appreciate any and all feedback!
Resolves #5210