icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

Consider changing PluralCategory discriminants to be 0 for Other

Open sffc opened this issue 1 year ago • 3 comments

In https://github.com/unicode-org/icu4x/pull/4496 I added ULE discriminants that match the Serde discriminants in PluralCategory, which is 0 == Zero. Although that is cute, I think we should try to stick with 0 == the undefined state, which is Other. At this point it would be a breaking change to any data struct that currently uses PluralCategory so it should be made in 2.0.

If we get too busy in 2.0 this is an easy thing to drop.

sffc avatar Dec 27 '23 17:12 sffc

This is a data struct bump, so I think it's independent of 2.0?

robertbastian avatar Jan 04 '24 10:01 robertbastian

True since we want to keep supporting V1 data structs then I guess this can be decoupled from 2.0

sffc avatar Jan 04 '24 10:01 sffc

Discuss with:

  • @sffc
  • @zbraniecki
  • @robertbastian
  • @echeran

sffc avatar Feb 01 '24 18:02 sffc

I don't think this is worth the cost of having a second data struct, which comes with a second code path for construction. This increases code size, and complexity in general.

robertbastian avatar Feb 28 '24 15:02 robertbastian

  • @sffc - Ideally I think that discriminant 0 should be the default/auto value.
  • @robertbastian - I don't see the issue, Rust users are not casting the enum to u8, and nobody is inspecting our raw ULE bytes. The change is churn
  • @Manishearth I'm ok with doing this if we do it without forking the data structs. Which is unlikely to happen.
  • @echeran - We go back and forth between default values and enums in CodePointTrie, for example, and we've had issues with the default values in that crate.
  • noted: it's also in the ABI as Zero = 0 for ICU4XPluralCategory

Decision: no change

robertbastian avatar Mar 07 '24 19:03 robertbastian