[NaN] handle NaN token as identifier #475
Conversation
Codecov Report
@@ Coverage Diff @@
## master #475 +/- ##
==========================================
- Coverage 66.46% 66.45% -0.01%
==========================================
Files 147 149 +2
Lines 9408 9421 +13
==========================================
+ Hits 6253 6261 +8
- Misses 3155 3160 +5
Continue to review full report at Codecov.
|
boa/src/builtins/nan/mod.rs
Outdated
| /// Initialise the `NaN` property on the global object. | ||
| #[inline] | ||
| pub fn init(global: &Value) { | ||
| let _timer = BoaProfiler::global().start_event("NaN", "init"); | ||
| global.set_field("NaN", Value::from(f64::NAN)); | ||
| } |
There was a problem hiding this comment.
I think its best to abstract it in a NaN struct and have this static method, like we do in number/mod.rs so we don't have free functions, and then call NaN::init(global) in init function in builtins/mod.rs
There was a problem hiding this comment.
I actually wen't to implement it like that at first, but seemed like bloating the code. I then checked other globals and some do it like this and some don't (50-50). Are you sure this is beneficial. Is the init of for example String or Math struct really an inherent part of the struct, or just external binding of it to the global object?
There was a problem hiding this comment.
We have other global functions related with numbers like parseInt in number, so maybe it makes sense to just add this there: https://github.com/boa-dev/boa/blob/a78934d424f17bc8b9132fdb14704b61e240bd40/boa/src/builtins/number/mod.rs
There was a problem hiding this comment.
@Razican Hmm. Number should be solely for the global object Number. Also these are not even the same functions in some cases. For example (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isNaN):
The Number.isNaN() method determines whether the passed value is NaN and its type is Number. It is a more robust version of the original, global isNaN().
But on the other hand the global NaN is the same as Number.NaN so we should reuse that logic somehow.
There was a problem hiding this comment.
@Razican Hmm. Number should be solely for the global object
Number.
This makes sense
Also these are not even the same functions in some cases.
I was talking about parseInt() and parseFloat(), that were recently added. Maybe we should have a "global" object initialization, and we should put all the global things in there.
Actually, we already have it, it's the global initialization function, where we call all those init functions, so we should probably add them in that outer function.
There was a problem hiding this comment.
If you ask me I would remove all these init functions as all of them are one-liners binding the objects on the provided global object.
I would just put in here "boa/src/builtins/mod.rs", something like this:
pub fn init(global: &Value) {
global.set_field("Function", Function::new());
global.set_field("BigInt", BigInt::new());
...
// instead of
//BigInt::init(global);
//Function::init(global);
// ...
}```There was a problem hiding this comment.
If you ask me I would remove all these
initfunctions as all of them are one-liners binding the objects on the provided global object.I would just put in here "boa/src/builtins/mod.rs", something like this:
pub fn init(global: &Value) { global.set_field("Function", Function::new()); global.set_field("BigInt", BigInt::new()); ... // instead of //BigInt::init(global); //Function::init(global); // ... }```
Yes, I think this is a good idea. We should have an InitBuiltin trait, that could be implemented by external libraries too, that just have an init() function and return a list of <&str, Value> that we would then add to the global object, maybe in parallel (or at least they could be generated in parallel).
Razican
added
builtins
|
We should also uncomment this test: boa/boa/src/builtins/array/tests.rs Line 455 in 4beadfc |
3 participants
#421