-
-
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
Make environment methods take &mut Context
#1131
Conversation
Test262 conformance changes:
|
8de49b4
to
9c88f59
Compare
Benchmark for 3437d38Click to view benchmark
|
Benchmark for 24ea698Click to view benchmark
|
Benchmark for e3a0c10Click to view benchmark
|
9c88f59
to
fa6e4d1
Compare
Benchmark for cce28a5Click to view benchmark
|
fa6e4d1
to
f52c86f
Compare
Benchmark for 9b446ceClick to view benchmark
|
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.
Just confused about the switch to todo!()
, the rest looks good.
Benchmark for cf586e1Click to view benchmark
|
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'm OK with these changes, but since it's adding 14 new panics and makes 6 extra tests fail, I would need a bit more of rationale to understand why this change is needed / useful for us.
Sorry, I should have explained this before 😅 The environment methods needs to take
We went around this problem by using |
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.
The code looks more idiomatic, for sure. Hopefully we can solve the extra panics and new failing tests in a new PR. I'm happy with it as-is.
80b7272
to
e47bd59
Compare
Benchmark for 247066fClick to view benchmark
|
It changes the following:
ErrorKind
LexicalEnvironment
methods toContext
&mut Context
Result<T>
pub(crate)