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 support for to_string and implement PlainDate::to_string #153

Merged
merged 9 commits into from
Jan 11, 2025

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Jan 7, 2025

This PR implements the underlying code to implement Display, to_string, and to_ixdtf_string methods for nearly all the main built-ins aside from Duration, PlainYearMonth, and PlainMonthDay.

The majority of the backing implementation will most likely need to eventually be moved into the ixdtf crate, but until that is complete and worked, this should allow us to move forward with the implementation and testing in Boa against test262.

@nekevss nekevss added the C-enhancement New feature or request label Jan 7, 2025
Copy link

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Praise: Thanks for using writeable :)

write_nanosecond(self.nanosecond, self.precision, sink)?;

Ok(())
}
Copy link

Choose a reason for hiding this comment

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

Suggestion: your impl Writeable may benefit from a write_len function since it looks fairly cheap to pre-compute. You could add one later.

src/parsers.rs Outdated
Comment on lines 91 to 93
if nanoseconds > 1_000_000_000 {
return Err(core::fmt::Error);
}
Copy link

Choose a reason for hiding this comment

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

Nit: You should avoid returning core::fmt::Error because it gets turned into an unhandled panic! in most call sites. You should try and ensure that your nanoseconds are in range by adding invariants to the type being formatted, instead of returning an error in the Writeable impl.

If you need to return errors from the Writeable impl, consider using TryWriteable instead.

src/parsers.rs Outdated
Comment on lines 104 to 111
let mut divisor = 100_000_000;
while divisor >= 1 && nanoseconds != 0 {
(nanoseconds / divisor).write_to(sink)?;
nanoseconds %= divisor;
divisor /= 10;
}

Ok(())
Copy link

Choose a reason for hiding this comment

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

Observation: The compiler can often optimize out a small fixed operation; for example, I think it changes /10 from an expensive div instruction to some clever bit shifting stuff. However, since the divisor is changing on each iteration of the loop, the compiler is less likely to do this optimization and may leave around an actual div instruction in the assembly.

You should consider making a little helper that returns [u8; 9] with the ASCII bytes of a padded 9-digit number, which you can fill in backwards to get the /10 optimization, and then write the subslice that you need from it into the sink. You can use it in all of these places where you need to write a padded number to a string. Unfortunately this may involve a small amount of unsafe code since you need to assert that the temporary [u8; 9] buffer is UTF-8. However, hopefully once core::ascii gets stabilized, you will make such code safe.

I may even support adding the helper function to writeable itself, but you should first develop it here.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good

src/parsers.rs Outdated
pub fn write_u32_to_ascii_digits(mut value: u32) -> ([u8; 9], usize) {
let mut output = [0; 9];
let mut precision = 0;
// let mut precision_check = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// let mut precision_check = 0;

src/parsers.rs Outdated
include_sep: true,
},
};
assert_eq!(offset.to_string(), "+04:00");
Copy link

Choose a reason for hiding this comment

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

Nit: Use assert_writeable_eq instead of assert_eq in order to test the Writeable impl more thoroughly (such as the correctness of writeable_length_hint)

Comment on lines +98 to +103
fn write_padded_u8<W: core::fmt::Write + ?Sized>(num: u8, sink: &mut W) -> core::fmt::Result {
if num < 10 {
sink.write_char('0')?;
}
num.write_to(sink)
}
Copy link

Choose a reason for hiding this comment

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

Suggestion: I like how you use impl Writeable for u8 here. I think you can do that with the u32 as well.

Something like:

let mut max = 1e9;
while max > nanoseconds {
    sink.write_char('0')?;
    max /= 10;
}
nanoseconds.write_to(sink)?;

And then you don't need any unsafe code.

Copy link
Member Author

@nekevss nekevss Jan 11, 2025

Choose a reason for hiding this comment

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

Does that fix output the nanoseconds according to what would be expected? If I'm reading that write, it would left pad nanoseconds to always be 9 digits, but the concern with this is to trim trailing zeros on nanosecond according to the desired precision.

nanoseconds = 123_000_000, should only print .123 or the desired precision, i.e. precision = 5 would be .12300, but this would print 123000000

The idea for the use of unsafe in the function was to try and optimize out the div instruction from the below, which I think would be the equivalent to the above.

   let mut divisor = 1_000_000_000;
    while divisor >= 1 && nanoseconds != 0 {
        (nanoseconds / divisor).write_to(sink)?;
        nanoseconds %= divisor;
        divisor /= 10;
    }

EDIT: Although another option may be to deconstruct and then reconstruct the u32? That may be an option to avoid unsafe

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, nevermind. We can just write the digits manually as u8s

@nekevss nekevss merged commit 436b07d into main Jan 11, 2025
5 checks passed
@nekevss nekevss deleted the date-to-string branch January 11, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants