-
Notifications
You must be signed in to change notification settings - Fork 454
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
Rename spectest imports to avoid depending on overloading. #652
Conversation
Well, Travis should never go red on master, so it'd be good if you could also adjust the interpreter (should be a straightforward change to function host/spectest.ml:lookup). Also, the suite now no longer tests that the "overloading" pattern at least has to validate. In order to avoid regressing coverage, could you add a simple test that checks that? (Could be done with |
I've now added a test for the "overloading" pattern, including overloading on arity, return types, and kinds. The interpreter code is not straightforward to me. |
@sunfishcode I can take a look at the interpreter part if you like. |
@binji Thanks! |
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.
Thanks @binji!
Thank you as well; I just ran into this hiccup in my own implementation. Any idea when this might get merged? |
In my earlier commits, I mapped the 0-argument "print" to "print_hello", however @binji's interpreter changes leave it named "print". It's not important which name is picked, so just update the one test which uses it to match the interpreter.
The JS test harness contains its own implementation of the spectest module; update it to reflect the new names for `print` and `global`.
I just added two more minor patches here; one is a trivial rename to synchronize the testsuite names with the names now used in the interpreter, and the other updates the JS test harness, which I just noticed also has an implementation 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.
Still LGTM
This has unanimous consent and various review approvals, and I'm not aware of any remaining outstanding issues. Merging, as discussed in the CG meeting today. |
This just updates the testsuite, and does not update the interpreter.