-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Merge CodeBlock
constant pools
#3413
Conversation
Test262 conformance changes
|
b7e55a2
to
1d9cf6c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3413 +/- ##
==========================================
- Coverage 45.64% 45.40% -0.24%
==========================================
Files 483 483
Lines 49517 49790 +273
==========================================
+ Hits 22600 22609 +9
- Misses 26917 27181 +264
☔ View full report in Codecov by Sentry. |
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.
Overall, this looks pretty good to me. Just had one thing regarding docs.
Definitely interested to see the new codeblock trace! 😄
boa_engine/src/vm/code_block.rs
Outdated
return value.clone(); | ||
} | ||
|
||
panic!("there should be a string constant at index {index}") |
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.
Can there be Panic
docs added to these functions so they pass future lints.
Also, a little nitpicky aside, I don't dislike naming the functions as _expect
. I do think it's clear, but would it be worthwhile being more explicit with _or_panic
? I'm fine with either/or but wanted to at least mention it.
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.
Maybe we should remove the postfix? I only added it because we have .resolve_expect()
and .environment_expect()
to be consistent with the naming. But there isn't a rust guideline neither does the std
lib do this.
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.
+1 to the suggestion about removing the suffix. It's not common for Rust APIs to name panicking functions with a suffix. It's much more common to panic by default, then offer try_*
variants for non-panicking functions.
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.
Yeah, I was sort of thinking about the try_*
naming convention when I was looking at it. It's much more common for us to return a JsResult
throughout the engine as a default. So instead of adding try_*
everywhere, adding some sort of naming convention on the panicking functions in our case might be a bit better clarity-wise.
That being said, sticking to the typical naming convention and removing the suffix is also a valid option.
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.
Look good. I only have one very minor suggestion.
eacc048
to
a2a8472
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.
This looks good to me 😄 Thanks for adding the panic docs!
* Merge `CodeBlock` constant pools * Apply review
This PR merges the
literals
,names
,functions
,compile_environments
constant pools into one constant pool calledconstants