Use pattern in `icu_decimal` #5385

robertbastian · 2024-08-16T11:31:37Z

robertbastian · 2024-08-16T11:33:08Z

utils/pattern/src/frontend/mod.rs

 use core::{
    convert::Infallible,
    fmt::{self, Write},
    marker::PhantomData,
 };
 use writeable::{adapters::TryWriteableInfallibleAsWriteable, PartsWrite, TryWriteable, Writeable};

+#[allow(clippy::unwrap_used, clippy::indexing_slicing)]
+impl<Store> SinglePlaceholderPattern<Store> where Store: Deref<Target = str> {


@sffc how do I do this nicely? Are there encoding helpers?

Use Pattern::try_from_items and then Pattern::take_store

robertbastian · 2024-08-16T11:34:29Z

components/decimal/src/provider.rs

    #[cfg_attr(feature = "serde", serde(borrow))]
-    pub minus_sign_affixes: AffixesV1<'data>,
+    pub minus_sign_pattern: Option<SinglePlaceholderPattern<Cow<'data, str>>>,


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

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

sffc

There is also #4437; I had been thinking that #4437 would be resolved and also solve the Pattern problem at the same time, but this is fine as a first step.

sffc · 2024-08-19T16:10:19Z

components/decimal/src/provider.rs

    #[cfg_attr(feature = "serde", serde(borrow))]
-    pub minus_sign_affixes: AffixesV1<'data>,
+    pub minus_sign_pattern: Option<SinglePlaceholderPattern<Cow<'data, str>>>,


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.

sffc · 2024-08-19T16:15:45Z

utils/pattern/src/frontend/mod.rs

+impl SinglePlaceholderPattern<str> {
+    pub fn encode_store(prefix: &str, suffix: &str) -> alloc::boxed::Box<str> {
+        alloc::format!(
+            "{}{prefix}{suffix}",
+            char::from_u32(prefix.len() as u32 + 1).unwrap()
+        )
+        .into_boxed_str()
+    }
+}


Issue: I would prefer to avoid adding methods directly to SinglePlaceholderPattern<str>. Methods should be added to Pattern<Store>.

Besides that, this code duplicates logic that lives in the pattern constructor. You should build the pattern using the standard APIs and then use take_store.

The constructor you probably want is Pattern::try_from_items.

sffc · 2024-08-19T16:20:31Z

utils/pattern/src/frontend/mod.rs

+#[allow(clippy::unwrap_used, clippy::indexing_slicing)]
+impl<Store> SinglePlaceholderPattern<Store>
+where
+    Store: Deref<Target = str>,


Issue: The bound I use everywhere else is AsRef<str>.

I don't like that bound.

We should open a discussion about whether we want the backend to be

AsRef<str>

Deref<Target = str>

Borrow<str>

str

But for now please remain consistent.

Backend::Store

Yes, for all of those I mean a substitution of str with Backend::Store

Just to start the commentary a bit:

I went with AsRef<Backend::Store> because it is the most flexible.

Why AsRef: The Store parameter worked out well for me in LiteMap and ZeroTrie before embarking on Pattern. It makes certain impls a pain to write, but once they are written, things "just work" and are very easy to use and very flexible.

Why not Deref: I don't care that the store dereferences uniquely to Backend::Store. For instance, at some point we might get Box<AsciiStr> which derefs to AsciiStr before str.

Why not Borrow: I don't care about the additional invariants on Borrow like Hash and Eq. If I want to implement those traits on Pattern, I can; I don't need Borrow's help.

Why not DSTs: I didn't go with the dynamically sized type because I dislike the amount of unsafe code required for the DSTs to work. I have made multiple attempts to write safe abstractions for ref casts and box casts, but they keep being shot down (#5101). I would like to block the pure DST approach on first solving #2778 / #2310 / #5127, but I'm open to it assuming it can be written in a way that minimizes unsafe.

sffc · 2024-08-19T16:23:39Z

utils/pattern/src/frontend/mod.rs

+        if let Some(index) = index.checked_sub(1) {
+            &chars.as_str()[index..]
+        } else {
+            chars.as_str()


Issue: this returns the same string for both prefix and suffix if there is no placeholder. I think suffix() should return Option<&str>.

utils/pattern/src/frontend/mod.rs

sffc · 2024-08-19T16:26:15Z

utils/pattern/src/frontend/mod.rs

+        let index = chars.next().unwrap() as usize;
+        if let Some(index) = index.checked_sub(1) {
+            &chars.as_str()[..index]
+        } else {
+            chars.as_str()
+        }


Issue: this code is not panic-proof. There are constructors that are not unsafe; you can make a Pattern pointing to an invalid store, and the docs say "unexpected behavior may occur" but not "panics may occur".

Please resolve the two fallible array getters in this function to debug-assert and then return "".

I know, that's why I asked how to do this properly

sffc · 2024-08-19T16:26:58Z

utils/pattern/src/frontend/mod.rs

+        let index = chars.next().unwrap() as usize;
+        if let Some(index) = index.checked_sub(1) {
+            &chars.as_str()[index..]


Issue: Please change these two fallible array getters to debug-assert and then return either None or Some("").

sffc · 2024-08-19T16:27:55Z

components/decimal/src/provider.rs

    #[cfg_attr(feature = "serde", serde(borrow))]
-    pub minus_sign_affixes: AffixesV1<'data>,
+    pub minus_sign_pattern: Option<SinglePlaceholderPattern<Cow<'data, str>>>,


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

sffc · 2024-08-20T06:34:59Z

It's also worth noting that the "correct" way to use Pattern is to use its own interpolation and construction APIs. Right now you are going out of your way to hack around the functions that Pattern offers to you on a silver platter. You wrote your own constructor and your own accessor functions (prefix and suffix), instead of using try_from_items and interpolate, so you are in effect using Pattern only as a storage mechanism.

Pattern, like ListFormatter, makes use of nested writeables. Kartavya did a good job with nested writeables in #5383. You can do this here by making a private type UnsignedFormattedFixedDecimal and then passing it to interpolate.

robertbastian · 2024-08-20T06:46:42Z

so you are in effect using Pattern only as a storage mechanism.

I don't think there's anything wrong with this.

You can do this here by making a private type UnsignedFormattedFixedDecimal and then passing it to interpolate.

I can, but this will increase code complexity and very likely also code size.

sffc · 2024-08-20T06:50:47Z

I think you're overestimating the code complexity of nested writeables. I'm quite happy with how it went in Kartavya's open PR and it reduces the total number of lines of code.

I don't think it will have an impact on code size, assuming everything is sufficiently inlined.

But this is purely a code cleanup thing, not a data representation thing, so it doesn't block the PR which is more about the data representation.

robertbastian · 2024-08-20T15:48:07Z

Discussion: we'll pursue stuffing all the strings in a multi-field-VarULE instead

decimal

ddcabb4

robertbastian force-pushed the decimal branch from 9c1051b to ddcabb4 Compare August 16, 2024 11:32

robertbastian commented Aug 16, 2024

View reviewed changes

fix

25e28e3

robertbastian marked this pull request as ready for review August 19, 2024 14:48

robertbastian requested review from zbraniecki, sffc, Manishearth and a team as code owners August 19, 2024 14:48

robertbastian added 2 commits August 19, 2024 16:58

fix

0b0db4f

size tests

bc0572e

robertbastian requested a review from nordzilla as a code owner August 19, 2024 15:21

sffc requested changes Aug 19, 2024

View reviewed changes

robertbastian removed request for zbraniecki, Manishearth and nordzilla August 19, 2024 20:50

robertbastian marked this pull request as draft August 19, 2024 20:50

robertbastian closed this Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pattern in `icu_decimal` #5385

Use pattern in `icu_decimal` #5385

robertbastian commented Aug 16, 2024

robertbastian Aug 16, 2024

sffc Aug 19, 2024

robertbastian Aug 16, 2024

sffc Aug 19, 2024

sffc left a comment

sffc Aug 19, 2024

sffc Aug 19, 2024

sffc Aug 19, 2024

robertbastian Aug 19, 2024

sffc Aug 19, 2024

robertbastian Aug 19, 2024

sffc Aug 20, 2024

sffc Aug 20, 2024

sffc Aug 19, 2024

sffc Aug 19, 2024

robertbastian Aug 19, 2024

sffc Aug 19, 2024

sffc Aug 19, 2024

sffc commented Aug 20, 2024

robertbastian commented Aug 20, 2024

sffc commented Aug 20, 2024

robertbastian commented Aug 20, 2024

Use pattern in icu_decimal #5385

Use pattern in icu_decimal #5385

Conversation

robertbastian commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Aug 20, 2024

robertbastian commented Aug 20, 2024

sffc commented Aug 20, 2024

robertbastian commented Aug 20, 2024

Use pattern in `icu_decimal` #5385

Use pattern in `icu_decimal` #5385