Implement toString #381
Implement toString #381
Conversation
|
I implemented the initial version but the problem is i made some mistake while translating c++ to c (to further use c2rust) and in some cases produces wrong values for example |
boa/src/builtins/number/mod.rs
Outdated
| // Insert decimal point. | ||
| let fresh0 = fraction_cursor; | ||
| fraction_cursor += 1; | ||
| buffer[fresh0] = b'.'; |
There was a problem hiding this comment.
In Rust, this indexing is very slow, since it will do bounds checking every time it accesses the buffer. I think we can somehow iterate through it using iterators instead of direct access, but then, the carry-overs can be problematic. Maybe @HalidOdat has some ideas on how to improve this performance.
There was a problem hiding this comment.
Something that can be done here to at least increase the iteration for the integer part is this:
let (int_buf, frac_buf) = buffer.split_at_mut(BUF_SIZE / 2);Then, in the integer iteration:
let mut int_iter = int_buf.into_iter().rev();
for c in int_iter {
let remainder = integer % (radix as f64);
*c = std::char::from_digit(remainder as u32, radix).unwrap() as u8;
integer = (integer - remainder) / radix as f64;
if integer <= 0_f64 {
break;
}
}
// Add sign and terminate string.
if negative {
*int_buf.next().expect("buffer ended") = b'-';
}This might need some mut-s somewhere around, and we need to save the index somehow, maybe by calling enumerate() in the iterator, but it should work. Maybe we can do something similar with the fractional part.
There was a problem hiding this comment.
We may also be able to replace some of the while loop using take_while.
There was a problem hiding this comment.
Fraction part is hard to refactor since there is a second pass to the buffer and implementation sometimes increments and sometimes decrements the cursor. Maybe we can do something with Rev but it will be complicated.
There was a problem hiding this comment.
Something like bidir_iter might work better but problem is they use slice.get internally so we still pay for the bound checks. We might use unsafe get to not pay for the bound check since we check the bounds anyway.
There was a problem hiding this comment.
I think it's good enough for now :) we can maybe improve it further in the future.
|
Ok using |
…[radix] ) Implement the radix paramet for the `toString`. This implementation is converted from the V8's c++ implementation.
Initial version for getting rid of direct slice accesses. Currently converted integer part to iterators. Fraction part is a lot harder since there are two passes to the fraction part (for carry over) and it is hard to express that using iterators.
boa/src/builtins/number/tests.rs
Outdated
| forward(&mut engine, "Number(0.0123).toString()") | ||
| ); | ||
| // assert_eq!(String::from("111111111111111110000"), forward(&mut engine, "Number(111111111111111111111).toString()")); | ||
| // assert_eq!(String::from("1.1111111111111111e+21"), forward(&mut engine, "Number(1111111111111111111111).toString()")); |
There was a problem hiding this comment.
This tests fails i think because rusts default formatting does not match the js spec.
There was a problem hiding this comment.
This tests fails i think because rusts default formatting does not match the js spec.
Yes. The rust formatter is not JavaScript spec compliant. Even using format!("{:e}", num) isn't sufficient.
We probably need to write our own f64 parser, like we did the one with bases. Here is V8 DoubleToCString
|
I was trying to resolve the conflicts and now it seems every number is interpreted as NaN. I dont it is a issue from the to_string method i just debug the value after calling to_number on it and it just prints NaN. |
|
Ok my bad i apparently called to_number on this without calling to_num |
boa/src/builtins/number/tests.rs
Outdated
| assert_eq!(default_string, String::from("0")); | ||
| assert_eq!(int_string, String::from("123")); | ||
| assert_eq!(float_string, String::from("1.234")); | ||
| assert_eq!(exp_string, String::from("12000")); | ||
| assert_eq!(neg_string, String::from("-1.2")); | ||
| assert_eq!(nan_string, String::from("NaN")); |
There was a problem hiding this comment.
I think we should make this easier to understand like this:
| assert_eq!(default_string, String::from("0")); | |
| assert_eq!(int_string, String::from("123")); | |
| assert_eq!(float_string, String::from("1.234")); | |
| assert_eq!(exp_string, String::from("12000")); | |
| assert_eq!(neg_string, String::from("-1.2")); | |
| assert_eq!(nan_string, String::from("NaN")); | |
| assert_eq!(&default_string, "0"); | |
| assert_eq!(&int_string, "123"); | |
| assert_eq!(&float_string, "1.234"); | |
| assert_eq!(&exp_string, "12000"); | |
| assert_eq!(&neg_string, "-1.2"); | |
| assert_eq!(&nan_string, "NaN"); |
I think we should do this in all tests.
boa/src/builtins/number/tests.rs
Outdated
| // assert_eq!(String::from("1e-7"), forward(&mut engine, "Number(0.0000001).toString()")); | ||
| // assert_eq!(String::from("1.2e-7"), forward(&mut engine, "Number(0.00000012).toString()")); | ||
| // assert_eq!(String::from("1.23e-7"), forward(&mut engine, "Number(0.000000123).toString()")); | ||
| // assert_eq!(String::from("1e-8"), forward(&mut engine, "Number(0.00000001).toString()")); | ||
| // assert_eq!(String::from("1.2e-8"), forward(&mut engine, "Number(0.000000012).toString()")); | ||
| // assert_eq!(String::from("1.23e-8"), forward(&mut engine, "Number(0.0000000123).toString()")); |
There was a problem hiding this comment.
I'm guessing these tests don't pass wither, right? If that's the case, I would isolate them in a different test and add the #[ignore] attribute to it, to know that we must fix them in the future.
boa/src/builtins/number/tests.rs
Outdated
| String::from("NaN"), | ||
| forward(&mut engine, "Number(NaN).toString(16)") |
There was a problem hiding this comment.
I would change all these to have this syntax:
| String::from("NaN"), | |
| forward(&mut engine, "Number(NaN).toString(16)") | |
| "NaN", | |
| &forward(&mut engine, "Number(NaN).toString(16)") |
No description provided.