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

Use pattern in icu_decimal #5385

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions components/decimal/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ all-features = true
displaydoc = { workspace = true }
fixed_decimal = { workspace = true }
icu_provider = { workspace = true, features = ["macros"] }
icu_pattern = { workspace = true, features = ["zerofrom"]}
writeable = { workspace = true }

databake = { workspace = true, features = ["derive"], optional = true}
Expand All @@ -46,8 +47,8 @@ criterion = { workspace = true }
[features]
default = ["compiled_data"]
std = ["fixed_decimal/std", "icu_locale_core/std", "icu_provider/std"]
serde = ["dep:serde", "icu_provider/serde"]
datagen = ["serde", "dep:databake"]
serde = ["dep:serde", "icu_provider/serde", "icu_pattern/serde"]
datagen = ["serde", "dep:databake", "icu_pattern/databake"]
bench = ["serde"]
compiled_data = ["dep:icu_decimal_data"]

Expand Down
18 changes: 10 additions & 8 deletions components/decimal/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
use crate::grouper;
use crate::options::*;
use crate::provider::*;
use alloc::borrow::Cow;
use fixed_decimal::FixedDecimal;
use fixed_decimal::Sign;
use icu_pattern::SinglePlaceholderPattern;
use writeable::Writeable;

/// An intermediate structure returned by [`FixedDecimalFormatter`](crate::FixedDecimalFormatter).
Expand All @@ -21,11 +23,11 @@ pub struct FormattedFixedDecimal<'l> {
}

impl<'l> FormattedFixedDecimal<'l> {
fn get_affixes(&self) -> Option<&AffixesV1> {
fn get_patterns(&self) -> Option<&SinglePlaceholderPattern<Cow<'l, str>>> {
match self.value.sign() {
Sign::None => None,
Sign::Negative => Some(&self.symbols.minus_sign_affixes),
Sign::Positive => Some(&self.symbols.plus_sign_affixes),
Sign::Negative => Some(self.symbols.minus_sign_pattern.as_ref().unwrap_or(&NEGATIVE_DEFAULT)),
Sign::Positive => Some(self.symbols.plus_sign_pattern.as_ref().unwrap_or(&POSITIVE_DEFAULT)),
}
}
}
Expand All @@ -35,9 +37,9 @@ impl<'l> Writeable for FormattedFixedDecimal<'l> {
where
W: core::fmt::Write + ?Sized,
{
let affixes = self.get_affixes();
if let Some(affixes) = affixes {
sink.write_str(&affixes.prefix)?;
let patterns = self.get_patterns();
if let Some(patterns) = patterns {
sink.write_str(&patterns.prefix())?;
}
let range = self.value.magnitude_range();
let upper_magnitude = *range.end();
Expand All @@ -56,8 +58,8 @@ impl<'l> Writeable for FormattedFixedDecimal<'l> {
sink.write_str(&self.symbols.grouping_separator)?;
}
}
if let Some(affixes) = affixes {
sink.write_str(&affixes.suffix)?;
if let Some(patterns) = patterns {
sink.write_str(&patterns.suffix())?;
}
Ok(())
}
Expand Down
48 changes: 12 additions & 36 deletions components/decimal/src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

use alloc::borrow::Cow;
use icu_provider::prelude::*;
use icu_pattern::SinglePlaceholderPattern;

#[cfg(feature = "compiled_data")]
#[derive(Debug)]
Expand Down Expand Up @@ -46,30 +47,6 @@ const _: () = {
/// The latest minimum set of markers required by this component.
pub const MARKERS: &[DataMarkerInfo] = &[DecimalSymbolsV1Marker::INFO];

/// A collection of strings to affix to a decimal number.
///
/// <div class="stab unstable">
/// 🚧 This code is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. While the serde representation of data structs is guaranteed
/// to be stable, their Rust representation might not be. Use with caution.
/// </div>
#[derive(Debug, PartialEq, Clone, yoke::Yokeable, zerofrom::ZeroFrom)]
#[cfg_attr(
feature = "datagen",
derive(serde::Serialize, databake::Bake),
databake(path = icu_decimal::provider),
)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
pub struct AffixesV1<'data> {
/// String to prepend before the decimal number.
#[cfg_attr(feature = "serde", serde(borrow))]
pub prefix: Cow<'data, str>,

/// String to append after the decimal number.
#[cfg_attr(feature = "serde", serde(borrow))]
pub suffix: Cow<'data, str>,
}

/// A collection of settings expressing where to put grouping separators in a decimal number.
/// For example, `1,000,000` has two grouping separators, positioned along every 3 digits.
///
Expand Down Expand Up @@ -117,13 +94,13 @@ pub struct GroupingSizesV1 {
)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
pub struct DecimalSymbolsV1<'data> {
/// Prefix and suffix to apply when a negative sign is needed.
/// Pattern to apply when a negative sign is needed.
#[cfg_attr(feature = "serde", serde(borrow))]
pub minus_sign_affixes: AffixesV1<'data>,
pub minus_sign_pattern: Option<SinglePlaceholderPattern<Cow<'data, str>>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

strictly speaking this should use a pattern type that requires exactly one placeholder

Copy link
Member

Choose a reason for hiding this comment

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

When you make .suffix() return an Option, you can debug_assert that it is Some.

Copy link
Member

Choose a reason for hiding this comment

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

Question: why did you make it an Option? The value is only 2 bytes long. Seems like unnecessary indirection? You already have a Cow so the niche is already being used.


/// Prefix and suffix to apply when a plus sign is needed.
/// Pattern to apply when a plus sign is needed.
#[cfg_attr(feature = "serde", serde(borrow))]
pub plus_sign_affixes: AffixesV1<'data>,
pub plus_sign_pattern: Option<SinglePlaceholderPattern<Cow<'data, str>>>,

/// Character used to separate the integer and fraction parts of the number.
#[cfg_attr(feature = "serde", serde(borrow))]
Expand All @@ -141,17 +118,16 @@ pub struct DecimalSymbolsV1<'data> {
pub digits: [char; 10],
}

/// The value used if [`DecimalSymbolsV1::minus_sign_pattern`] is empty.
pub static NEGATIVE_DEFAULT: SinglePlaceholderPattern<Cow<'static, str>> = SinglePlaceholderPattern::from_store_unchecked(Cow::Borrowed("\x02-"));
/// The value used if [`DecimalSymbolsV1::plus_sign_pattern`] is empty.
pub static POSITIVE_DEFAULT: SinglePlaceholderPattern<Cow<'static, str>> = SinglePlaceholderPattern::from_store_unchecked(Cow::Borrowed("\x02+"));

impl Default for DecimalSymbolsV1<'static> {
fn default() -> Self {
Self {
minus_sign_affixes: AffixesV1 {
prefix: Cow::Borrowed("-"),
suffix: Cow::Borrowed(""),
},
plus_sign_affixes: AffixesV1 {
prefix: Cow::Borrowed("+"),
suffix: Cow::Borrowed(""),
},
minus_sign_pattern: None,
plus_sign_pattern: None,
decimal_separator: ".".into(),
grouping_separator: ",".into(),
grouping_sizes: GroupingSizesV1 {
Expand Down
4 changes: 2 additions & 2 deletions ffi/capi/src/decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ pub mod ffi {
>(
icu_provider::DataPayload::<DecimalSymbolsV1Marker>::from_owned(
DecimalSymbolsV1 {
plus_sign_affixes,
minus_sign_affixes,
plus_sign_pattern,
minus_sign_pattern,
decimal_separator: str_to_cow(decimal_separator),
grouping_separator: str_to_cow(grouping_separator),
grouping_sizes,
Expand Down

Large diffs are not rendered by default.

Loading
Loading