Skip to content
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

Feature/try catch #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

hath995
Copy link

@hath995 hath995 commented May 1, 2016

Tried to implement try, catch, finally. It captures the correct control flow. I added some helper methods for testing as well as some tweaks so I could run the tests using nodejs and mocha.

I haven't quite got the behavior right on the attaching and populating the scope needed for the catch block. Edit: grabbed the right scope code for catch blocks from master and added it to this implementation, and it fixed the last test.

@NeilFraser
Copy link
Owner

I don't understand how this differs from our existing try/catch/finally implementation. For instance, you add a stepCatchClause function at line 2644, overwriting the one already at line 2163. Does the existing implementation have bugs?

@hath995
Copy link
Author

hath995 commented May 1, 2016

I started this a couple months back when it wasn't implemented, then put it down for a while. I pulled from master and saw a stepTry and stepCatch had been implemented and I thought I would post my implementation in case it was useful. It seems like it was too late.

@hath995
Copy link
Author

hath995 commented May 1, 2016

A related question I have is, do you have a test harness that you're using with the interpreter or are you manually testing bit of code?

@hath995
Copy link
Author

hath995 commented May 1, 2016

Actually, I branched off master and added in my test harness and many tests break. It looks like the cases of returning or continuing from within in a try/catch block are not handled correctly. There was also an infinite loop in a test of rethrowing.

@NeilFraser
Copy link
Owner

The tests I use are the unit tests for other projects. If one can load the other project and run their own unit tests, then that exercises the interpreter pretty well. My regular tests are from my other projects, Diff Match Patch, and the Blockly generators. But any projects are good, as long as they don't rely on DOM methods.

@hath995
Copy link
Author

hath995 commented May 6, 2016

I asked just because without the tests available internally, I think it's difficult to confidently contribute. Implementing try/catch/finally, was pretty tricky so I grabbed a lot of these tests from https://github.com/tc39/test262/tree/es5-tests and they covered plenty of weird situations which I would not have thought to cover initially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants