implement RegExp.prototype [ @@search ] ( string ) #1314
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test262 conformance changes:
Fixed tests: |
RageKnify
suggested changes
Jun 11, 2021
* RegExp.prototype [ @@search ] ( string ) * String.prototype.search ( regexp )
* RegExp.prototype [ @@search ] ( string ) * String.prototype.search ( regexp )
|
@RageKnify Thanks for the review and the great explanations! I fixed all the things you mentioned. I refactored the code to include the spec as comments. Please let me know if that is desired in this codebase. Further I added some very basic tests, but I'm not sure if this is required, as the full 262 test suite should cover everything? |
Test262 results look very good now! 54 fixed tests :) Using the spec as comments in the code is a good idea, that makes everything easier to follow. I think it's also good to have built-in tests. These will be executed faster, so it's easier to check for regressions. Having basic tests (or internal implementation tests) is good from my side. I will review the code during the weekend. |
LinusU
reviewed
Jun 14, 2021
boa/src/builtins/regexp/mod.rs
Outdated
| pub(crate) fn search(this: &Value, args: &[Value], context: &mut Context) -> Result<Value> { | ||
| // 1. Let rx be the this value. | ||
| // 2. If Type(rx) is not Object, throw a TypeError exception. | ||
| if let Some(object) = this.as_object() { |
There was a problem hiding this comment.
If you want to avoid indenting the rest of the function you can use match to either unwrap or early return, something like this:
let object = match this.as_object() {
Some(object) => object,
None => return context.throw_type_error("RegExp.prototype.search method called on incompatible value"),
}ref: https://doc.rust-lang.org/rust-by-example/error/result/early_returns.html
There was a problem hiding this comment.
You can also use if !this.is_object() and then expect() on the as_object().
Razican
added
enhancement
|
It seems that this requires running |
* Add some more rust tests from the 262 suite
RageKnify
approved these changes
Jun 14, 2021
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request implements part of #1304 .
It changes the following:
ECMASCript Test262 test suite results: