-
-
Notifications
You must be signed in to change notification settings - Fork 401
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
Number constants #420
Number constants #420
Conversation
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! Check my comments to see on how we can improve it :)
Co-authored-by: HalidOdat <[email protected]>
Co-authored-by: HalidOdat <[email protected]>
Thanks for the help, changes made :) With regards to Number.NaN vs Number.NAN is this something which should be looked at as part of this or separately? (unless I've made a mistake of course). |
About this, it should be treated separately, it would require changes to the lexer and parser. For now you can comment it out and add a |
boa/src/builtins/number/mod.rs
Outdated
// Constants from: | ||
// Draft ECMA-262 / May 22, 2020 ECMAScript® 2021 Language Specification | ||
// Section 20.1.2 Properties of the Number Constructor |
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 we change this for a link?
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.
Removed section and added link
boa/src/builtins/number/mod.rs
Outdated
number.set_field("POSITIVE_INFINITY", Value::from(f64::INFINITY)); | ||
|
||
// TODO: Currently parsing/lexing Number.NaN is not supported due to the inclusion of the lower case 'a'. | ||
// number.set_field("NaN", Value::from(f64::NAN)); |
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 would still add this here, so that when we fix the lexer this will just work. What do you think, @HalidOdat?
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 would still add this here, so that when we fix the lexer this will just work. What do you think, @HalidOdat?
Yep. We could add it! That's probably the best option.
Edit: we might be able to access it with Number["NaN"]
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.
Will add it back :)
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 created #421 related to 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.
This is good to go for me, thanks for the work!
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.
Looks good to me too :)
This Pull Request fixes #414 .
It changes the following:
The constant NaN is expressed currently as NAN because using the lowercase a was causing the parser to interpret it as an identifier not a constant.