-
-
Notifications
You must be signed in to change notification settings - Fork 420
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 "Boa" to boa_engine, moved GC and profiler to their crates #1844
Conversation
f6c6126
to
c595aa5
Compare
Test262 conformance changesVM implementation
|
Codecov Report
@@ Coverage Diff @@
## main #1844 +/- ##
==========================================
+ Coverage 55.27% 56.09% +0.81%
==========================================
Files 200 200
Lines 17860 17901 +41
==========================================
+ Hits 9873 10042 +169
+ Misses 7987 7859 -128
Continue to review full report at Codecov.
|
Unfortunately, the benchmarking error won't go away, since the path for the main crate is different in the PR and in the "main" branch. |
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.
No comments from my side :)
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.
Looks good to me.
Just the fixes for boa_profiler
.
boa_profiler/Cargo.toml
Outdated
] | ||
|
||
[dependencies] | ||
measureme = { version = "10.0.0", optional = true } |
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.
measureme = { version = "10.0.0", optional = true } | |
measureme = { version = "10.0.0", optional = true } | |
once_cell = "1.9.0" |
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 decided to add a feature, in order to not have an unused dependency when the measure feature was not selected.
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.
Besides the things pointed out by @raskad. Everything looks good from my side :)
I had a query around the use of “engine”. I think this was already discussed so sorry for bringing it back up. But why did we go with engine over core? I’d assume the fact it’s a js engine is implied, and core would be more consistent with both ChakraCore and JavascriptCore the Microsoft and WebKit engines respectively. For me engine is a bit like having “interpreter” in the name. |
@jasonwilliams my argument was that it seems weird for |
7ab2913
to
d5c2e6b
Compare
016aec0
to
07adca6
Compare
This PR closes #230.
It changes the following:
Position
in multiple places, or we were importing theTryInto
trait, which is now in the prelude)I didn't move the Boa console to its own crate, since this requires further work in the engine, since it is currently part of the
Context
. Ideally, I would like this to be a separate crate and for "boa_engine" to not have it as a dependency ("boa_cli" would depend on it, of course, maybe "boa_wasm" too).