DurationFormatter: Remove allocations #5383
Conversation
and add testdata
add documentation to hour and minute format
previously using incorrect function to set maximum fractional digits
| let mut list_sink = PartSink::new(); | ||
| formatted_list.write_to_parts(&mut list_sink)?; | ||
|
|
||
| // 7. Let strings be a new empty List. |
There was a problem hiding this comment.
keep these comments and adjust them with the new implementation
There was a problem hiding this comment.
They are redundant; I believe @sffc said that it was acceptable to have multiple part annotations from different formatters in the final output. (Let me know if I misunderstood or misquoted you)
The comments are responsible for stripping list formatter parts information from the output.
We could add a divergence from standard note here. What do you think?
There was a problem hiding this comment.
It's probably good to move some of the spec line item comments to where they are done in the new code structure. But you don't need a 1-for-1 alignment. The code is spec-compliant so long as the behavior is the same; it doesn't need to be line-for-line the same.
| @@ -235,6 +235,7 @@ diplomat-tool = { git = "https://github.com/rust-diplomat/diplomat", rev = "8744 | |||
| # EXTRA_CAPI_DEPS | |||
| # EXTRA_BLOB_DEPS | |||
| # EXTRA_FS_DEPS | |||
| arrayvec = { version = "0.7.2", default-features = false } | |||
There was a problem hiding this comment.
issue: we already have smallvec in this list, can you use that?
There was a problem hiding this comment.
I think arrayvec is slightly better than smallvec in this instance because there is a known, small upper limit to the length of the vector. smallvec has code and branching to overflow to the heap, which we don't need here.
arrayvec is in Cargo.lock so it's not a new dependency.
There was a problem hiding this comment.
It is not currently a runtime dep: https://github.com/unicode-org/icu4x/blob/main/tools/make/depcheck/src/allowlist.rs#L41
There was a problem hiding this comment.
True. Flagging @Manishearth for the new runtime dependency.
There was a problem hiding this comment.
If we're adding another crate on top of smallvec, I think it should be heapless, which has more features than arrayvec. I'm not convinced this is worth adding another crate though.
| let mut list_sink = PartSink::new(); | ||
| formatted_list.write_to_parts(&mut list_sink)?; | ||
|
|
||
| // 7. Let strings be a new empty List. |
There was a problem hiding this comment.
It's probably good to move some of the spec line item comments to where they are done in the new code structure. But you don't need a 1-for-1 alignment. The code is spec-compliant so long as the behavior is the same; it doesn't need to be line-for-line the same.
follow up to #5321