Implement `try_from_bytes` for Pattern #5034

younies · 2024-06-11T13:12:42Z

No description provided.

utils/pattern/src/multi_named.rs

utils/pattern/src/common.rs

utils/pattern/src/multi_named.rs

Co-authored-by: Shane F. Carr <shane@unicode.org>

sffc · 2024-06-12T12:55:52Z

utils/pattern/src/frontend/mod.rs

+    pub fn try_from_utf8(bytes: &[u8]) -> Result<Self, Error> {
+        let store = B::try_store_from_utf8(bytes).map_err(|_| Error::InvalidPattern)?;
+        #[cfg(debug_assertions)]
+        match B::validate_store(core::borrow::Borrow::borrow(&store)) {
+            Ok(()) => (),
+            Err(e) => {
+                debug_assert!(false, "Pattern validation failed: {:?}", e);
+            }
+        };
+
+        Ok(Self::from_store_unchecked(store.to_owned()))
+    }


Issue: You should always call validate_store here, not just in debug_assertions

done, but why this is not the case in try_from_items ?

Because you are constructing from a store that you haven't validated yet

but try_store_from_utf8 has already validated it

sffc · 2024-06-12T12:59:56Z

utils/pattern/src/common.rs

@@ -101,6 +103,10 @@ pub trait PatternBackend: crate::private::Sealed + 'static {
    where
        Self::Store: ToOwned;

+    /// Converts a byte slice store to this pattern backend's store.
+    #[doc(hidden)] // TODO(#4467): Should be internal
+    fn try_store_from_utf8(utf8: &[u8]) -> Result<&Self::Store, Utf8Error>;


Issue: This should return a trait type, not always Utf8Error. You should add a new associated type, as I had in my suggestion from earlier.

yes, I added StoreUtf8Error

sffc · 2024-06-12T15:37:26Z

utils/pattern/src/double.rs

@@ -270,6 +270,7 @@ impl PatternBackend for DoublePlaceholder {
    #[cfg(feature = "alloc")]
    type PlaceholderKeyCow<'a> = DoublePlaceholderKey;
    type Error<'a> = Infallible;
+    type StoreUtf8Error = StoreUtf8Error;


Issue: it should return just Utf8Error

There is no way to convert PatternError to Utf8Error or even to creat an instance of Utf8Error

try_store_from_utf8 should do exactly 1 thing, which is convert [u8] to the generic type Store. If you want it to also validate the pattern, then the function should be named validate_store_utf8 or similar. But I suggested try_store_from_utf8 since it is the minimal operation that we need.

sffc

Objectively, you're calling validate_store twice, which is a bug. I suggested an approach to fix it.

utils/pattern/src/error.rs

utils/pattern/src/single.rs

utils/pattern/src/multi_named.rs

utils/pattern/src/frontend/mod.rs

utils/pattern/src/double.rs

Co-authored-by: Shane F. Carr <shane@unicode.org>

utils/pattern/src/lib.rs

Co-authored-by: Shane F. Carr <shane@unicode.org>

sffc

The code is good! Thank you!

I'm having second thoughts on some names, but we can merge this and bikeshed names later.

sffc · 2024-06-13T16:23:31Z

utils/pattern/src/frontend/mod.rs

+{
+    /// Creates a pattern from a UTF-8 encoded byte slice.
+    ///
+    /// ✨ *Enabled with the `alloc` Cargo feature.*


Nit: It does not need the alloc feature

sffc · 2024-06-13T19:05:48Z

utils/pattern/src/frontend/mod.rs

@@ -2,27 +2,27 @@
 // called LICENSE at the top level of the ICU4X source tree
 // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

+extern crate alloc;


Oops, you shouldn't need this. The bounds on your new impl are wrong. They should be on Store, not Store::Owned.

I tried as much as I can, could you fix it please

…to dev-pattern-try-from-u8

sffc · 2024-06-14T05:17:28Z

OK I pushed 9180c26 which fixed the alloc issue

Then I pushed a few more commits to change the names. I realized that this isn't the right context to use the name utf8 since we don't guarantee that all patterns will forever have utf-8 backing stores.

younies · 2024-06-14T10:45:40Z

OK I pushed 9180c26 which fixed the alloc issue

Then I pushed a few more commits to change the names. I realized that this isn't the right context to use the name utf8 since we don't guarantee that all patterns will forever have utf-8 backing stores.

Okay, thanks a lot

I have pushed small commit to fix the doc, it can be merged now

Implement try_from_utf8 for Pattern

0cf3bc5

younies requested review from sffc and zbraniecki June 11, 2024 13:12

younies added 4 commits June 11, 2024 15:30

implement the todos

1bad04f

correct function signature

e512d74

try_implementation

b707a62

add test

b717506

younies commented Jun 12, 2024

View reviewed changes

utils/pattern/src/multi_named.rs Outdated Show resolved Hide resolved

sffc reviewed Jun 12, 2024

View reviewed changes

utils/pattern/src/common.rs Outdated Show resolved Hide resolved

utils/pattern/src/multi_named.rs Outdated Show resolved Hide resolved

younies and others added 3 commits June 12, 2024 14:20

Update utils/pattern/src/multi_named.rs

56ae16d

Co-authored-by: Shane F. Carr <shane@unicode.org>

Update utils/pattern/src/common.rs

b4e443b

Co-authored-by: Shane F. Carr <shane@unicode.org>

try to fix

5c86550

sffc reviewed Jun 12, 2024

View reviewed changes

younies added 6 commits June 12, 2024 15:25

Fix

d6a4c95

First fix

bf10868

Fix all the issues

b1bbb80

fix clippy

9ccde40

fix fmt

643d959

Merge branch 'main' into dev-pattern-try-from-u8

9c7c9f0

younies marked this pull request as ready for review June 12, 2024 14:15

younies requested a review from sffc June 12, 2024 14:15

younies added 2 commits June 12, 2024 16:17

always validate store

7025c8b

fix typo

95c8a1b

sffc reviewed Jun 12, 2024

View reviewed changes

younies requested a review from sffc June 12, 2024 16:05

sffc requested changes Jun 13, 2024

View reviewed changes

younies and others added 3 commits June 13, 2024 11:16

Update utils/pattern/src/error.rs

eeda8c0

Co-authored-by: Shane F. Carr <shane@unicode.org>

Update utils/pattern/src/single.rs

5462cf0

Co-authored-by: Shane F. Carr <shane@unicode.org>

Update utils/pattern/src/multi_named.rs

f5d3093

Co-authored-by: Shane F. Carr <shane@unicode.org>

younies added 2 commits June 13, 2024 16:06

fix CI

774d8e4

Merge branch 'main' into dev-pattern-try-from-u8

4288cdc

sffc reviewed Jun 13, 2024

View reviewed changes

utils/pattern/src/lib.rs Outdated Show resolved Hide resolved

younies and others added 3 commits June 13, 2024 17:16

Update utils/pattern/src/lib.rs

4a4420e

Co-authored-by: Shane F. Carr <shane@unicode.org>

fix CI

d7f93a5

fix ci

86e38ab

younies requested a review from sffc June 13, 2024 15:49

fix ci

142f641

sffc previously approved these changes Jun 13, 2024

View reviewed changes

sffc reviewed Jun 13, 2024

View reviewed changes

remove incorrect comment

e683dba

younies dismissed sffc’s stale review via e683dba June 13, 2024 18:01

fix ci

a79ed0c

younies requested a review from sffc June 13, 2024 18:09

sffc requested changes Jun 13, 2024

View reviewed changes

younies added 2 commits June 13, 2024 22:29

Merge branch 'dev-pattern-try-from-u8' of github.com:younies/icu4x in…

73b391d

…to dev-pattern-try-from-u8

Remove dependence on alloc

a4fd887

younies requested a review from sffc June 13, 2024 21:11

sffc added 5 commits June 14, 2024 08:07

Fix bounds

9180c26

Rename: try_from_bytes_store

972bd7a

Reorder function

001e38d

fmt

f9f53c3

Rename to "bytes" because the store is not guaranteed to be utf-8

919475e

sffc changed the title ~~Implement try_from_utf8 for Pattern~~ Implement try_from_bytes for Pattern Jun 14, 2024

fix doc

0b71296

sffc approved these changes Jun 14, 2024

View reviewed changes

younies merged commit 21efbae into unicode-org:main Jun 14, 2024
28 checks passed

younies deleted the dev-pattern-try-from-u8 branch June 27, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `try_from_bytes` for Pattern #5034

Implement `try_from_bytes` for Pattern #5034

younies commented Jun 11, 2024

sffc Jun 12, 2024

younies Jun 12, 2024

sffc Jun 12, 2024

younies Jun 12, 2024

sffc Jun 12, 2024

younies Jun 12, 2024

sffc Jun 12, 2024

younies Jun 12, 2024 •

edited

Loading

sffc Jun 13, 2024

younies Jun 13, 2024

sffc left a comment

sffc left a comment

sffc Jun 13, 2024

younies Jun 13, 2024

sffc Jun 13, 2024

younies Jun 13, 2024

sffc commented Jun 14, 2024

younies commented Jun 14, 2024

Implement try_from_bytes for Pattern #5034

Implement try_from_bytes for Pattern #5034

Conversation

younies commented Jun 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younies Jun 12, 2024 • edited Loading

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

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

sffc commented Jun 14, 2024

younies commented Jun 14, 2024

Implement `try_from_bytes` for Pattern #5034

Implement `try_from_bytes` for Pattern #5034

younies Jun 12, 2024 •

edited

Loading