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.

Sign up for GitHub

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

Jump to bottom

fix #209 "Calling Array with one argument" #328

Merged
merged 3 commits into from
Apr 29, 2020
Merged

fix #209 "Calling Array with one argument" #328

merged 3 commits into from
Apr 29, 2020

Conversation

HalidOdat
Copy link
Member

This is supposed to fix #209.

While trying to fix this bug I encountered another bug were we only get Const::Num and never Const::Int

This PR is blocked until it is fixed.

I will create another PR addressing that bug.

@HalidOdat HalidOdat added blocked Waiting for another code change bug Something isn't working labels Apr 17, 2020
@Razican
Copy link
Member

Razican commented Apr 17, 2020 •
edited
Loading

While trying to fix this bug I encountered another bug were we only get Const::Num and never Const::Int

This PR is blocked until it is fixed.

This was mentioned in https://github.com/jasonwilliams/boa/pull/270/files#r401575688

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 17, 2020 •
edited
Loading

While trying to fix this bug I encountered another bug were we only get Const::Num and never Const::Int
This PR is blocked until it is fixed.

This was mentioned in #270: #270 (files)

It seems that we never output a token of type i32 the only numerical value is of floating type f64.

I think v8 has a i32 token type smi (small intiger) right?

Edit: Yes they do v8 token type

I will change Token to have different numerical types.

@HalidOdat HalidOdat added this to the v0.8.0 milestone Apr 17, 2020
@HalidOdat HalidOdat mentioned this pull request Apr 17, 2020
Closed
@Razican
Copy link
Member

Razican commented Apr 18, 2020

I will change Token to have different numerical types.

Nice. I see that they also have a "big integer" is this part of the spec in some way?

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 18, 2020 •
edited
Loading

I will change Token to have different numerical types.

Nice. I see that they also have a "big integer" is this part of the spec in some way?

Yes. I checked the specs for NumericLiteral which is a part of Token and BigInt is one of the numerical types.

@HalidOdat HalidOdat mentioned this pull request Apr 19, 2020
Merged
@HalidOdat HalidOdat removed the blocked Waiting for another code change label Apr 28, 2020
@HalidOdat HalidOdat force-pushed the fix/array-with-one-argument branch from 901560f to 93cceee Compare April 29, 2020 00:44
@HalidOdat HalidOdat marked this pull request as ready for review April 29, 2020 00:45
Razican
Razican approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks very good to me. Check my comments just in case, but this is ready :)

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
HalidOdat and others added 2 commits April 29, 2020 19:15
@Razican Razican merged commit dd0f967 into boa-dev:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Razican Razican Razican approved these changes

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.

Calling new Array with 1 argument doesn't work properly
2 participants
@HalidOdat @Razican