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.

Sign up for GitHub

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

Jump to bottom

FixedDecimal *ed methods should be infallible #4857

Closed
sffc opened this issue May 2, 2024 · 5 comments · Fixed by #5623
Closed

FixedDecimal *ed methods should be infallible #4857

sffc opened this issue May 2, 2024 · 5 comments · Fixed by #5623
Labels
2.0-breaking Changes that are breaking API changes C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement)
Milestone
ICU4X 2.0 ⟨P1⟩

Comments

@sffc
Copy link
Member

sffc commented May 2, 2024

This function is misleading:

pub fn concatenated_end(self, other: FixedDecimal) -> Result<Self, FixedDecimal>

In the error case, it returns other, but it still throws away self. It should probably return a tuple containing both FixedDecimals. It makes it tempting to write code such as mantissa.concatenated_end(fraction).unwrap_or_else(|fd| fd), which is wrong because it promotes fraction when it should probably keep mantissa and throw away fraction.

I think we should delete this function since it is only sugar over concatenate_end, which is harder to use incorrectly.
@sffc sffc added C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement) 2.0-breaking Changes that are breaking API changes labels May 2, 2024
@sffc sffc added this to the ICU4X 2.0 milestone May 2, 2024
@sffc sffc mentioned this issue May 2, 2024
Merged
@robertbastian
Copy link
Member

For the datetime use case, I think it would be useful to have a FixedDecimal constructor that accepts usize, usize as the integer and fractional parts. Then we avoid this infallibility altogether.

@sffc
Copy link
Member Author

sffc commented May 2, 2024

For the datetime use case, I think it would be useful to have a FixedDecimal constructor that accepts usize, usize as the integer and fractional parts. Then we avoid this infallibility altogether.

I don't think it solves the overlap issue.

@robertbastian
Copy link
Member

I think it does. It would use IntIterator::from(integer).chain(IntIterator::from(fractional)) and then shift the resulting FixedDecimal by fractional.log10().

@sffc
Copy link
Member Author

sffc commented May 3, 2024

Hmm, that's quite clever. How does it handle these cases though?

  • Seconds = 123, Nanoseconds = 20 (needs to know that there are leading zeros: easy enough)
  • Seconds = 123, Nanoseconds = 4444444444 (same problem as we currently have: overlapping fields)

@sffc
Copy link
Member Author

sffc commented May 3, 2024

I wouldn't be opposed to implementing a plus operator that just performs a sum.

@Manishearth Manishearth mentioned this issue Sep 30, 2024
Merged
Manishearth added a commit that referenced this issue Oct 1, 2024
@Manishearth
Return Err((self, other)) in FixedDecimal::concatenated_end (#5623)
1d17017 
Fixes #4857

I think that's the only `fn *ed* -> Result` that we have.

<!--
Thank you for your pull request to ICU4X!

Reminder: try to use [Conventional
Comments](https://conventionalcomments.org/) to make comments clearer.

Please see
https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for
general
information on contributing to ICU4X.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
2.0-breaking Changes that are breaking API changes C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement)
Projects
icu4x 2.0
Status: Done
Milestone
ICU4X 2.0 ⟨P1⟩
Development

Successfully merging a pull request may close this issue.

Return Err((self, other)) in FixedDecimal::concatenated_end Manishearth/icu4x
2 participants
@sffc @robertbastian