Skip to content
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

implement "this" #320

Merged
merged 6 commits into from
May 16, 2020
Merged

implement "this" #320

merged 6 commits into from
May 16, 2020

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented Apr 15, 2020

@jasonwilliams jasonwilliams added the help wanted Extra attention is needed label Apr 15, 2020
@github-actions
Copy link

Benchmark for cc1a196

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 367.1±22.30µs 374.0±25.40µs 98%
Expression (Parser) 2.8±0.17µs 2.8±0.16µs 101%
Hello World (Execution) 399.5±23.07µs 394.6±22.58µs 101%
Hello World (Lexer) 876.3±67.47ns 878.6±47.91ns 100%
Hello World (Parser) 1048.1±86.55ns 1059.3±71.45ns 99%
Symbol Creation 468.5±30.27µs 463.0±28.00µs 101%
fibonacci (Execution) 4.6±0.31ms 4.4±0.32ms 103%
undefined undefined 100%

@Razican
Copy link
Member

Razican commented Apr 16, 2020

Hmmm I will give this a look during the weekend and see if I find out how it can be done :)

@Razican
Copy link
Member

Razican commented May 7, 2020

This should no longer be blocked on function objects.

About the environment trait, for now, I think this is good, but we might want to have multiple traits, depending on what should each environment have. For example, a global environment shouldn't have a parent environment, so it doesn't need to implement that function (and we avoid the Option there). But I think this is out of the scope of this PR.

@Razican Razican changed the title implement this fixes #264 implement this May 7, 2020
@Razican Razican changed the title implement this implement "this" May 7, 2020
@Razican Razican added the enhancement New feature or request label May 7, 2020
@jasonwilliams jasonwilliams marked this pull request as ready for review May 13, 2020 16:14
@jasonwilliams
Copy link
Member Author

This is ready for review, @Razican clippy is moaning about https://github.com/jasonwilliams/boa/pull/320/files#diff-d7bd296d9ca534161e198b8c141ca51fR611 im not sure what to do about it.

@github-actions
Copy link

Benchmark for 43a9df7

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 345.9±22.82µs 349.1±18.06µs 99%
Expression (Lexer) 1740.9±96.04ns 1695.5±108.56ns 103%
Expression (Parser) 4.5±0.23µs 4.3±0.28µs 103%
Fibonacci (Execution) 2.5±0.12ms 2.6±0.11ms 97%
For loop (Execution) 368.8±17.51µs 391.4±25.63µs 94%
For loop (Lexer) 4.5±0.22µs 4.7±0.25µs 96%
For loop (Parser) 12.6±0.76µs 12.6±0.64µs 100%
Hello World (Lexer) 872.7±51.67ns 806.6±41.14ns 108%
Hello World (Parser) 2.0±0.10µs 2.1±0.12µs 95%
Symbols (Execution) 378.9±18.55µs 383.7±20.92µs 99%
undefined undefined 100%

@jasonwilliams jasonwilliams added this to the v0.8.0 milestone May 13, 2020
@Razican
Copy link
Member

Razican commented May 13, 2020

This is ready for review, @Razican clippy is moaning about https://github.com/jasonwilliams/boa/pull/320/files#diff-d7bd296d9ca534161e198b8c141ca51fR611 im not sure what to do about it.

Let me check this tomorrow or on Friday :)

/// Utility to create a function Value for Function Declarations, Arrow Functions or Function Expressions
pub(crate) fn create_function(
&mut self,
args: &Box<[FormalParameter]>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we have two options here. Either we receive a Box<[FormalParameter]> or a &[FormalParameter]. If you take a Box, you can directly pass it to FunctionObject::create_ordinary and this would probably reflect better how the function works internally, but you would need to clone it on call.

If this is not possible, you can just receive a &[FormalParameter] and then do a to_vec().into_boxed_slice(), or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will try both options later

@github-actions
Copy link

Benchmark for f9b7032

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 406.8±14.16µs 420.5±29.90µs 97%
Expression (Lexer) 1995.8±92.86ns 2.1±0.10µs 97%
Expression (Parser) 5.0±0.19µs 5.1±0.38µs 98%
Fibonacci (Execution) 2.8±0.09ms 3.1±0.09ms 92%
For loop (Execution) 431.4±16.74µs 450.1±17.78µs 96%
For loop (Lexer) 5.3±0.23µs 5.5±0.36µs 97%
For loop (Parser) 15.1±0.71µs 14.9±0.59µs 102%
Hello World (Lexer) 996.7±40.67ns 1017.6±46.54ns 98%
Hello World (Parser) 2.5±0.11µs 2.5±0.11µs 100%
Symbols (Execution) 449.6±22.97µs 445.5±16.72µs 101%
undefined undefined 100%

@jasonwilliams jasonwilliams merged commit 63f37a2 into master May 16, 2020
@HalidOdat HalidOdat mentioned this pull request May 16, 2020
@HalidOdat HalidOdat deleted the implement_this branch May 31, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deal with [[Call]], [[Constuct]] duplication Implement the "this" keyword
2 participants