-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
[Merged by Bors] - Implement Async Generators #2200
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2200 +/- ##
==========================================
- Coverage 41.99% 41.54% -0.46%
==========================================
Files 231 234 +3
Lines 21530 21891 +361
==========================================
+ Hits 9042 9094 +52
- Misses 12488 12797 +309
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
VM implementation
|
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.
This is great! I started the review, but still a lot to go through.
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.
This looks really good!
I just have some small nitpicks :)
Currently async generator class methods are not being parsed correctly. In comparison to the current main branch this only fixes a few tests, but after #2200 is merged, it should be aroud 1000 additional passing tests.
b3afd4f
to
e11d9b0
Compare
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.
It looks great! I had some improvement suggestions, but in general this is great work :) and the coverage increase is impressive
boa_engine/src/bytecompiler.rs
Outdated
|
||
/// `FunctionCompiler` is used to compile AST functions to bytecode. | ||
#[derive(Debug, Clone, Copy)] | ||
pub(crate) struct FunctionCompiler { |
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.
I would move this struct to a sub-module. 3k lines is already too much in this file, and we also have #1808 that mentions dividing this huge file into smaller parts.
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.
True. I have moved the bytecompiler to a module folder and created a separate file for the function compiler. I think from here we can do further dividing.
bors r+ |
This Pull Request fixes #1560. It changes the following: - Implement `AsyncGeneratorFunction` builtin object. - Implement `AsyncGenerator` builtin object. - Implement async generator execution. - Add some parse errors for async generators. The `AsyncGenerator.prototype.return()` method currently does not work correctly with `finally` blocks, but I think we should merge this first to not increase the complexity of the pr too much.
Pull request successfully merged into main. Build succeeded: |
This Pull Request fixes #1560.
It changes the following:
AsyncGeneratorFunction
builtin object.AsyncGenerator
builtin object.The
AsyncGenerator.prototype.return()
method currently does not work correctly withfinally
blocks, but I think we should merge this first to not increase the complexity of the pr too much.