Support data for Bidi properties with combined data structure #3026

echeran · 2023-01-25T19:00:03Z

sffc · 2023-01-25T19:03:07Z

components/properties/src/props.rs

+#[allow(clippy::exhaustive_structs)] // newtype
+#[repr(transparent)]
+#[zerovec::make_ule(BidiMirroringGlyphULE)]
+pub struct BidiMirroringGlyph(pub u32);


Issue: We should make this a 3-byte type so that the data provider also stores it as 3 bytes.

[u8; 3] is one way to accomplish this. Alternatively, a cleaner approach would be to add UnvalidatedChar in the zerovec crate with the correct semantics.

I'd like to use the high 3 bits to fit more related properties in this data structure, so just "unvalidated char" wouldn't the the whole semantic. We might reuse the U24 struct that the normalizer had before.

Quoting self from Slack:

I do think it makes sense to bundle at least Bidi_Mirroring_Glyph, Bidi_Mirrored, Bidi_Paired_Bracket , and Bidi_Paired_Bracket_Type as a 24-bit value in one trie. I'm less sure of whether it makes sense to extend the trie value to 32 bits and bake Bidi_Class in the additional 8 bits.

components/properties/src/bidi_data.rs

Manishearth · 2023-03-03T23:19:13Z

components/properties/src/bidi_data.rs

+///
+
+
+/// 20..0  Code point return value for Bidi_Mirroring_Glyph value


these docs should go on the ULE type

Also, this module should probably be a submodule of provider and marked unstable the way we usually do provider code, but not doc(hidden)

also these ranges seem backwards?

Manishearth · 2023-03-03T23:21:23Z

components/properties/src/bidi_data.rs

+            //
+            // TODO: is the above statement correct and okay?
+            // TODO: or do we have a way to validate the enum discriminant against the enum for
+            //       newtype idiom enums?


issue: we must validate that all unused bytes are 0 (or some other fixed value)

components/properties/src/bidi_data.rs

Co-authored-by: Manish Goregaokar <[email protected]>

…ider module

sffc · 2023-03-23T08:59:20Z

provider/testdata/src/lib.rs

@@ -92,7 +92,7 @@ pub mod versions {
    /// # Examples
    ///
    /// ```
-    /// assert_eq!("icu4x/2023-02-24/72.x", icu_testdata::versions::icu_tag());
+    /// assert_eq!("icu4x/2023-03-22a/72.x", icu_testdata::versions::icu_tag());


Nit: I'd prefer for the tag change to be its own standalone PR (I've given similar feedback to others in the past)

I just remembered: in order for ci-job-testdata to pass, the tag needs to be updated because it refers to the data from the latest tag when running cargo make testdata. So if we want the latest version tag to be changed in a separate PR, then merging this PR will require overriding the fact that the relevant CI check will be failing (and that the follow on CI check full-datagen won't run as a result, too).

You can include the tag update in this PR but also pull it out, merge the other PR, and then update this PR to the latest main, which should be a small change. But I'll leave it up to you if you want to do this. It's a preference of mine but not super strong since this is only an additive change.

…ired_Bracket_Type in MirroredPairedBracketData

sffc · 2023-03-24T19:39:31Z

components/properties/src/bidi_data.rs

+            CheckedBidiPairedBracketType::Open => BidiPairingProperties::Open(mirroring_glyph),
+            CheckedBidiPairedBracketType::Close => BidiPairingProperties::Close(mirroring_glyph),


I think this is the place where you consume the CheckedBidiPairedBracketType, right? I don't understand the advantage of using the closed enum instead of using the open enum throughout.

We can't use the open enum throughout: it has states that cannot be represented in the ULE type because we don't have space.

If we want to use the open enum, MirroredPairedBracketData needs to have private fields, with a custom validating serde impl (and a custom ctor for databake).

Manishearth · 2023-03-24T19:57:53Z

components/properties/src/bidi_data.rs

+
+use icu_provider::prelude::*;
+
+/// A wrapper around certain Bidi properties data. Can be obtained via [`load_bidi_auxiliary_properties_unstable()`] and


thought (nb): Now that we have all these nice wrapper types I kinda feel like the load methods should have gone on those types instead of being free functions.

Might be a thing to do for 2.0? What do y'all think?

components/properties/src/bidi_data.rs

Manishearth · 2023-03-24T20:04:45Z

components/properties/src/props.rs

+#[cfg_attr(feature = "datagen", databake(path = icu_properties))]
+#[allow(clippy::exhaustive_structs)] // newtype
+#[repr(transparent)]
+#[zerovec::make_ule(BidiPairedBracketTypeULE)]


Yeah, it's necessary to be able to make CPTs for datagen. That said, we could cfg_attr(feature = "datagen) this, or just use CPT<u8> in datagen.

Manishearth · 2023-03-24T20:06:39Z

components/properties/src/props.rs

+pub struct BidiPairedBracketType(pub u8);
+
+#[allow(non_upper_case_globals)]
+impl BidiPairedBracketType {


thought: hmm, so I was under the impression we already had this type when I suggested making a closed and open.

If this is a new type, perhaps this can just be a closed enum for now (and we have a single type). @sffc thoughts?

this is being discussed below

components/properties/src/provider/bidi_data.rs

…BracketType

sffc · 2023-03-27T21:19:29Z

components/properties/src/error.rs

@@ -31,6 +31,9 @@ pub enum PropertiesError {
    /// An unknown or unexpected property name was used for an API dealing with properties specified as strings at runtime
    #[displaydoc("Unexpected or unknown property name")]
    UnexpectedPropertyName,
+    /// A `u32` serialized value of `MirroredPairedBracketData` did not encode either a valid Bidi_Mirroring_Glyph or a valid Bidi_Paired_Bracket_Type
+    #[displaydoc("Invalid MirroredPairedBracketData serialized in int: {0}")]
+    MirroredPairedBracketDataFromU32(u32),


Suggestion: I think slightly more idiomatic in Rust would be for this to be its own little error struct named MirroredPairedBracketDataTryFromError

…erter impls

Add newtype enum for Bidi_Mirroring_Glyph

a15e209

echeran requested a review from hsivonen January 25, 2023 19:00

sffc requested changes Jan 25, 2023

View reviewed changes

echeran added 2 commits February 27, 2023 17:08

Remove BidiMirroringGlyph enum, include BidiPairedBracketType enum

13bc755

Add module for Bidi data structure

8824b5b

echeran changed the title ~~Add newtype enum for Bidi_Mirroring_Glyph~~ Support data for Bidi properties with combined data structure Mar 3, 2023

echeran added 3 commits March 3, 2023 14:38

Fix bit shift off-by-one

36178cd

Code cleanup

c243114

Revert removal of unit test import

5e03d4d

Manishearth reviewed Mar 3, 2023

View reviewed changes

components/properties/src/bidi_data.rs Outdated Show resolved Hide resolved

Manishearth reviewed Mar 3, 2023

View reviewed changes

echeran and others added 17 commits March 4, 2023 00:08

Update components/properties/src/bidi_data.rs

34b9878

Co-authored-by: Manish Goregaokar <[email protected]>

Fix bit arithmetic notation when bit masking

4eb4acc

Address clippy suggestions

dbf2fbe

Merge branch 'main' into bmg-prop

9e77fb7

Apply clippy suggestions

55f79b0

Construct newtype enum directly using integer arg rather than via ULE

81ad049

Move docs on ULE bit layout to ULE type

7739224

Merge branch 'main' into bmg-prop

316483b

Apply review feedback

8b09582

Move special struct for Bidi property data to submodule of props prov…

f63c979

…ider module

Add data provider struct for Bidi property data

fef664c

Add user-facing struct where APIs will exist

55c2a7b

Create "-Borrowed" version of user-facing API substrate struct

b8c179e

Renaming data struct and user-facing API substrate struct

ea010d3

Rename struct field to match property name

7d69847

Add APIs to user-facing API substrate wrapper struct

9089b07

Add datagen DataProvider impl for BidiAuxiliaryPropertiesV1

82f9835

echeran mentioned this pull request Mar 19, 2023

Provide lookup for the Bidi_Mirroring_Glyph property #2833

Closed

Merge branch 'main' into bmg-prop

f8f1967

sffc reviewed Mar 23, 2023

View reviewed changes

echeran added 10 commits March 23, 2023 15:22

Minor formatting

3804d6d

Add new source data files to datagen testdata allowlist

e6f383e

Revert update to icuexportdata tag

7c2b3cf

Re-address clippy warnings due to feature-gated code

3f4e372

Apply more clippy changes

d8a8de1

Create closed enum to enforce ULE invariant of valid data for Bidi_Pa…

e15826a

…ired_Bracket_Type in MirroredPairedBracketData

Apply changes from cargo make testdata

79c2a78

Merge branch 'main' into bmg-prop

9f48443

Make the trie value struct MirroredPairedBracketData fields public

be133f2

Apply clippy linter changes

adc3f1b

echeran marked this pull request as ready for review March 24, 2023 19:30

echeran requested review from robertbastian and a team as code owners March 24, 2023 19:30

echeran requested a review from sffc March 24, 2023 19:30

sffc reviewed Mar 24, 2023

View reviewed changes

Manishearth reviewed Mar 24, 2023

View reviewed changes

echeran added 4 commits March 24, 2023 14:53

Merge branch 'main' into bmg-prop

fee095c

Add doc

71ff4a8

Replace open enum BidiPairedBracketType with closed CheckedBidiPaired…

6532cc9

…BracketType

Apply changes from cargo make diplomat-coverage

4088a3d

echeran requested a review from sffc March 25, 2023 18:18

sffc reviewed Mar 27, 2023

View reviewed changes

Creates error type MirroredPairedBracketDataFromU32, reorganizes conv…

cada203

…erter impls

sffc approved these changes Mar 27, 2023

View reviewed changes

This was linked to issues Mar 27, 2023

Provide lookup for the Bidi_Mirroring_Glyph property #2833

Closed

Provide data for Bidi pairing of brackets #3030

Closed

Manishearth approved these changes Mar 27, 2023

View reviewed changes

echeran merged commit b675c71 into unicode-org:main Mar 27, 2023

echeran deleted the bmg-prop branch March 27, 2023 23:01

		///


		/// 20..0 Code point return value for Bidi_Mirroring_Glyph value

		CheckedBidiPairedBracketType::Open => BidiPairingProperties::Open(mirroring_glyph),
		CheckedBidiPairedBracketType::Close => BidiPairingProperties::Close(mirroring_glyph),


		use icu_provider::prelude::*;

		/// A wrapper around certain Bidi properties data. Can be obtained via [`load_bidi_auxiliary_properties_unstable()`] and

Support data for Bidi properties with combined data structure #3026

Support data for Bidi properties with combined data structure #3026

Uh oh!

Conversation

echeran commented Jan 25, 2023 • edited by sffc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

echeran commented Jan 25, 2023 •

edited by sffc

Loading