Skip to content

Commit

Permalink
Consistently wrap all options bags in None in Rust and FFI (#6084)
Browse files Browse the repository at this point in the history
Fixes #5488


This does not touch experimental stuff. We should add this to the
graduation checklist.


<!--
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.
-->
  • Loading branch information
Manishearth authored Feb 7, 2025
1 parent b7d7729 commit a051c5b
Show file tree
Hide file tree
Showing 15 changed files with 71 additions and 88 deletions.
4 changes: 2 additions & 2 deletions components/casemap/src/casemapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ impl CaseMapper {
char_is_lead: impl Fn(&CaseMap, char) -> bool,
) -> StringAndWriteable<'a, FullCaseWriteable<'a, true>> {
let data = self.data.get();
let (head, rest) = match options.leading_adjustment {
let (head, rest) = match options.leading_adjustment.unwrap_or_default() {
LeadingAdjustment::Auto | LeadingAdjustment::ToCased => {
let first_cased = src.char_indices().find(|(_i, ch)| char_is_lead(data, *ch));
if let Some((first_cased, _ch)) = first_cased {
Expand All @@ -200,7 +200,7 @@ impl CaseMapper {
rest,
CaseMapLocale::from_langid(langid),
MappingKind::Title,
options.trailing_case,
options.trailing_case.unwrap_or_default(),
);
StringAndWriteable {
string: head,
Expand Down
20 changes: 12 additions & 8 deletions components/casemap/src/titlecase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use writeable::Writeable;
///
/// let default_options = Default::default();
/// let mut preserve_case: TitlecaseOptions = Default::default();
/// preserve_case.trailing_case = TrailingCase::Unchanged;
/// preserve_case.trailing_case = Some(TrailingCase::Unchanged);
///
/// // Exhibits trailing case when set:
/// assert_eq!(
Expand Down Expand Up @@ -74,8 +74,8 @@ pub enum TrailingCase {
/// let default_options = Default::default(); // head adjustment set to Auto
/// let mut no_adjust: TitlecaseOptions = Default::default();
/// let mut adjust_to_cased: TitlecaseOptions = Default::default();
/// no_adjust.leading_adjustment = LeadingAdjustment::None;
/// adjust_to_cased.leading_adjustment = LeadingAdjustment::ToCased;
/// no_adjust.leading_adjustment = Some(LeadingAdjustment::None);
/// adjust_to_cased.leading_adjustment = Some(LeadingAdjustment::ToCased);
///
/// // Exhibits leading adjustment when set:
/// assert_eq!(
Expand Down Expand Up @@ -147,10 +147,14 @@ pub enum LeadingAdjustment {
pub struct TitlecaseOptions {
/// How to handle the rest of the string once the head of the
/// string has been titlecased
pub trailing_case: TrailingCase,
///
/// Default is [`TrailingCase::Lower`]
pub trailing_case: Option<TrailingCase>,
/// Whether to start casing at the beginning of the string or at the first
/// relevant character.
pub leading_adjustment: LeadingAdjustment,
///
/// Default is [`LeadingAdjustment::Auto`]
pub leading_adjustment: Option<LeadingAdjustment>,
}

/// A wrapper around [`CaseMapper`] that can compute titlecasing stuff, and is able to load additional data
Expand Down Expand Up @@ -301,7 +305,7 @@ impl<CM: AsRef<CaseMapper>> TitlecaseMapper<CM> {
langid: &LanguageIdentifier,
options: TitlecaseOptions,
) -> impl Writeable + 'a {
if options.leading_adjustment == LeadingAdjustment::Auto {
if options.leading_adjustment.unwrap_or_default() == LeadingAdjustment::Auto {
// letter, number, symbol, or private use code point
const HEAD_GROUPS: GeneralCategoryGroup = GeneralCategoryGroup::Letter
.union(GeneralCategoryGroup::Number)
Expand Down Expand Up @@ -375,7 +379,7 @@ impl<CM: AsRef<CaseMapper>> TitlecaseMapper<CM> {
///
/// let default_options = Default::default();
/// let mut no_adjust: TitlecaseOptions = Default::default();
/// no_adjust.leading_adjustment = LeadingAdjustment::None;
/// no_adjust.leading_adjustment = Some(LeadingAdjustment::None);
///
/// // Exhibits leading adjustment when set:
/// assert_eq!(
Expand Down Expand Up @@ -415,7 +419,7 @@ impl<CM: AsRef<CaseMapper>> TitlecaseMapper<CM> {
///
/// let default_options = Default::default();
/// let mut preserve_case: TitlecaseOptions = Default::default();
/// preserve_case.trailing_case = TrailingCase::Unchanged;
/// preserve_case.trailing_case = Some(TrailingCase::Unchanged);
///
/// // Exhibits trailing case when set:
/// assert_eq!(
Expand Down
2 changes: 1 addition & 1 deletion components/datetime/src/format/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ mod tests {
];

let mut decimal_formatter_options = DecimalFormatterOptions::default();
decimal_formatter_options.grouping_strategy = GroupingStrategy::Never;
decimal_formatter_options.grouping_strategy = Some(GroupingStrategy::Never);
let decimal_formatter = DecimalFormatter::try_new(
icu_locale_core::locale!("en").into(),
decimal_formatter_options,
Expand Down
2 changes: 1 addition & 1 deletion components/datetime/src/pattern/names.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2202,7 +2202,7 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> {
return Ok(());
}
let mut options = DecimalFormatterOptions::default();
options.grouping_strategy = GroupingStrategy::Never;
options.grouping_strategy = Some(GroupingStrategy::Never);
self.decimal_formatter = Some(DecimalFormatterLoader::load(
loader,
(&prefs).into(),
Expand Down
2 changes: 1 addition & 1 deletion components/decimal/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl Writeable for FormattedDecimal<'_> {
if grouper::check(
upper_magnitude,
m,
self.options.grouping_strategy,
self.options.grouping_strategy.unwrap_or_default(),
self.symbols.grouping_sizes,
) {
w.with_part(parts::GROUP, |w| {
Expand Down
2 changes: 1 addition & 1 deletion components/decimal/src/grouper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ fn test_grouper() {
}
let provider = Provider(RefCell::new(Some(symbols)), digits);
let options = options::DecimalFormatterOptions {
grouping_strategy: cas.strategy,
grouping_strategy: Some(cas.strategy),
..Default::default()
};
let fdf =
Expand Down
19 changes: 9 additions & 10 deletions components/decimal/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@
#[non_exhaustive]
pub struct DecimalFormatterOptions {
/// When to render grouping separators.
pub grouping_strategy: GroupingStrategy,
///
/// Default is [`GroupingStrategy::Auto`]
pub grouping_strategy: Option<GroupingStrategy>,
}

impl From<GroupingStrategy> for DecimalFormatterOptions {
fn from(grouping_strategy: GroupingStrategy) -> Self {
Self { grouping_strategy }
Self {
grouping_strategy: Some(grouping_strategy),
}
}
}

Expand All @@ -31,7 +35,7 @@ impl From<GroupingStrategy> for DecimalFormatterOptions {
///
/// let locale = Default::default();
/// let mut options: options::DecimalFormatterOptions = Default::default();
/// options.grouping_strategy = options::GroupingStrategy::Min2;
/// options.grouping_strategy = Some(options::GroupingStrategy::Min2);
/// let df = DecimalFormatter::try_new(locale, options)
/// .expect("locale should be present");
///
Expand All @@ -42,9 +46,10 @@ impl From<GroupingStrategy> for DecimalFormatterOptions {
/// assert_writeable_eq!(df.format(&ten_thousand), "10,000");
/// ```
#[non_exhaustive]
#[derive(Debug, Eq, PartialEq, Clone, Copy, Hash)]
#[derive(Debug, Eq, PartialEq, Clone, Copy, Hash, Default)]
pub enum GroupingStrategy {
/// Render grouping separators according to locale preferences.
#[default]
Auto,

/// Never render grouping separators.
Expand All @@ -61,9 +66,3 @@ pub enum GroupingStrategy {
/// grouping separators, but numbers 10,000 and above will.
Min2,
}

impl Default for GroupingStrategy {
fn default() -> Self {
Self::Auto
}
}
2 changes: 2 additions & 0 deletions components/plurals/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#[non_exhaustive]
pub struct PluralRulesOptions {
/// Plural rule type to use.
///
/// Default is [`PluralRuleType::Cardinal`]
pub rule_type: Option<PluralRuleType>,
}

Expand Down
12 changes: 6 additions & 6 deletions components/segmenter/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ fn line_break_iter_latin1(c: &mut Criterion) {
let segmenter = LineSegmenter::new_dictionary(Default::default());

let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Anywhere;
options.word_option = LineBreakWordOption::BreakAll;
options.strictness = Some(LineBreakStrictness::Anywhere);
options.word_option = Some(LineBreakWordOption::BreakAll);
let segmenter_css = LineSegmenter::new_dictionary(options);

group.bench_function("En", |b| {
Expand Down Expand Up @@ -49,8 +49,8 @@ fn line_break_iter_utf8(c: &mut Criterion) {
let segmenter_dictionary = LineSegmenter::new_dictionary(Default::default());

let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Anywhere;
options.word_option = LineBreakWordOption::BreakAll;
options.strictness = Some(LineBreakStrictness::Anywhere);
options.word_option = Some(LineBreakWordOption::BreakAll);
let segmenter_css_dictionary = LineSegmenter::new_dictionary(options);

// No need to test "auto", "lstm", or "dictionary" constructor variants since English uses only
Expand Down Expand Up @@ -98,8 +98,8 @@ fn line_break_iter_utf16(c: &mut Criterion) {
let segmenter_dictionary = LineSegmenter::new_dictionary(Default::default());

let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Anywhere;
options.word_option = LineBreakWordOption::BreakAll;
options.strictness = Some(LineBreakStrictness::Anywhere);
options.word_option = Some(LineBreakWordOption::BreakAll);
let segmenter_css_dictionary = LineSegmenter::new_dictionary(options);

// No need to test "auto", "lstm", or "dictionary" constructor variants since English uses only
Expand Down
34 changes: 15 additions & 19 deletions components/segmenter/src/line.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const ZWJ: u8 = 54;
/// property values in the CSS Text spec. See the details in
/// <https://drafts.csswg.org/css-text-3/#line-break-property>.
#[non_exhaustive]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
#[derive(Copy, Clone, PartialEq, Eq, Debug, Default)]
pub enum LineBreakStrictness {
/// Breaks text using the least restrictive set of line-breaking rules.
/// Typically used for short lines, such as in newspapers.
Expand All @@ -152,6 +152,7 @@ pub enum LineBreakStrictness {
/// resolving class [CJ](https://www.unicode.org/reports/tr14/#CJ) to
/// [NS](https://www.unicode.org/reports/tr14/#NS);
/// see rule [LB1](https://www.unicode.org/reports/tr14/#LB1).
#[default]
Strict,

/// Breaks text assuming there is a soft wrap opportunity around every
Expand All @@ -168,10 +169,11 @@ pub enum LineBreakStrictness {
/// property values in the CSS Text spec. See the details in
/// <https://drafts.csswg.org/css-text-3/#word-break-property>
#[non_exhaustive]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
#[derive(Copy, Clone, PartialEq, Eq, Debug, Default)]
pub enum LineBreakWordOption {
/// Words break according to their customary rules. See the details in
/// <https://drafts.csswg.org/css-text-3/#valdef-word-break-normal>.
#[default]
Normal,

/// Breaking is allowed within "words".
Expand All @@ -185,13 +187,17 @@ pub enum LineBreakWordOption {

/// Options to tailor line-breaking behavior.
#[non_exhaustive]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
#[derive(Copy, Clone, PartialEq, Eq, Debug, Default)]
pub struct LineBreakOptions<'a> {
/// Strictness of line-breaking rules. See [`LineBreakStrictness`].
pub strictness: LineBreakStrictness,
///
/// Default is [`LineBreakStrictness::Strict`]
pub strictness: Option<LineBreakStrictness>,

/// Line break opportunities between letters. See [`LineBreakWordOption`].
pub word_option: LineBreakWordOption,
///
/// Default is [`LineBreakStrictness::Normal`]
pub word_option: Option<LineBreakWordOption>,

/// Content locale for line segmenter
///
Expand All @@ -202,16 +208,6 @@ pub struct LineBreakOptions<'a> {
pub content_locale: Option<&'a LanguageIdentifier>,
}

impl Default for LineBreakOptions<'_> {
fn default() -> Self {
Self {
strictness: LineBreakStrictness::Strict,
word_option: LineBreakWordOption::Normal,
content_locale: None,
}
}
}

#[derive(Debug)]
struct ResolvedLineBreakOptions {
strictness: LineBreakStrictness,
Expand All @@ -227,8 +223,8 @@ impl From<LineBreakOptions<'_>> for ResolvedLineBreakOptions {
false
};
Self {
strictness: options.strictness,
word_option: options.word_option,
strictness: options.strictness.unwrap_or_default(),
word_option: options.word_option.unwrap_or_default(),
ja_zh,
}
}
Expand Down Expand Up @@ -325,8 +321,8 @@ pub type LineBreakIteratorUtf16<'l, 's> = LineBreakIterator<'l, 's, LineBreakTyp
/// };
///
/// let mut options = LineBreakOptions::default();
/// options.strictness = LineBreakStrictness::Strict;
/// options.word_option = LineBreakWordOption::BreakAll;
/// options.strictness = Some(LineBreakStrictness::Strict);
/// options.word_option = Some(LineBreakWordOption::BreakAll);
/// options.content_locale = None;
/// let segmenter = LineSegmenter::new_auto(options);
///
Expand Down
16 changes: 8 additions & 8 deletions components/segmenter/tests/css_line_break.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,32 +32,32 @@ static JA: LanguageIdentifier = langid!("ja");

fn strict(s: &str, ja_zh: bool, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Strict;
options.word_option = LineBreakWordOption::Normal;
options.strictness = Some(LineBreakStrictness::Strict);
options.word_option = Some(LineBreakWordOption::Normal);
options.content_locale = ja_zh.then_some(&JA);
check_with_options(s, expect_utf8, expect_utf16, options);
}

fn normal(s: &str, ja_zh: bool, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Normal;
options.word_option = LineBreakWordOption::Normal;
options.strictness = Some(LineBreakStrictness::Normal);
options.word_option = Some(LineBreakWordOption::Normal);
options.content_locale = ja_zh.then_some(&JA);
check_with_options(s, expect_utf8, expect_utf16, options);
}

fn loose(s: &str, ja_zh: bool, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Loose;
options.word_option = LineBreakWordOption::Normal;
options.strictness = Some(LineBreakStrictness::Loose);
options.word_option = Some(LineBreakWordOption::Normal);
options.content_locale = ja_zh.then_some(&JA);
check_with_options(s, expect_utf8, expect_utf16, options);
}

fn anywhere(s: &str, ja_zh: bool, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Anywhere;
options.word_option = LineBreakWordOption::Normal;
options.strictness = Some(LineBreakStrictness::Anywhere);
options.word_option = Some(LineBreakWordOption::Normal);
options.content_locale = ja_zh.then_some(&JA);
check_with_options(s, expect_utf8, expect_utf16, options);
}
Expand Down
12 changes: 6 additions & 6 deletions components/segmenter/tests/css_word_break.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,24 @@ fn check_with_options(

fn break_all(s: &str, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Strict;
options.word_option = LineBreakWordOption::BreakAll;
options.strictness = Some(LineBreakStrictness::Strict);
options.word_option = Some(LineBreakWordOption::BreakAll);
options.content_locale = None;
check_with_options(s, expect_utf8, expect_utf16, options);
}

fn keep_all(s: &str, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Strict;
options.word_option = LineBreakWordOption::KeepAll;
options.strictness = Some(LineBreakStrictness::Strict);
options.word_option = Some(LineBreakWordOption::KeepAll);
options.content_locale = None;
check_with_options(s, expect_utf8, expect_utf16, options);
}

fn normal(s: &str, expect_utf8: Vec<usize>, expect_utf16: Vec<usize>) {
let mut options = LineBreakOptions::default();
options.strictness = LineBreakStrictness::Strict;
options.word_option = LineBreakWordOption::Normal;
options.strictness = Some(LineBreakStrictness::Strict);
options.word_option = Some(LineBreakWordOption::Normal);
options.content_locale = None;
check_with_options(s, expect_utf8, expect_utf16, options);
}
Expand Down
10 changes: 2 additions & 8 deletions ffi/capi/src/casemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,15 +326,9 @@ impl From<ffi::TitlecaseOptionsV1> for TitlecaseOptions {
fn from(other: ffi::TitlecaseOptionsV1) -> Self {
let mut ret = Self::default();

ret.leading_adjustment = other
.leading_adjustment
.into_converted_option()
.unwrap_or(ret.leading_adjustment);
ret.leading_adjustment = other.leading_adjustment.into_converted_option();

ret.trailing_case = other
.trailing_case
.into_converted_option()
.unwrap_or(ret.trailing_case);
ret.trailing_case = other.trailing_case.into_converted_option();

ret
}
Expand Down
Loading

0 comments on commit a051c5b

Please sign in to comment.