Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SignedPlainOldULE, add serialize impls for ULE types (also missing Eq impl for VZV) #1356

Closed
wants to merge 8 commits into from

Conversation

Manishearth
Copy link
Member

in ZeroVec the "non ULE" version is always available so we can always convert to/from it for ser/de. However, VZV deals with unsized types and as such does not know anything about the "non ULE" version (EncodeAsVarULE helps but only in specific contexts). We could use FromVarULE (#1180) here but FromVarULE will likely allow for multiple impls and there wouldn't be a way to select a particular one.

The current ser/de impls for VarZeroVec will serialize the byte buffer on machine formats, but for human readable formats will instead convert each element to Box<T> and serialize that instead.

For human readable serialization to do the "right thing", the serialization of the VarULE type should "look like the serialization of a normal type". This works out fine for regular VarULE types, however we have a complication when it comes to types like [T: ULE] because there can be more than one AsULE type that it matches. We would like VarZeroVec<[i32::ULE]> and VarZeroVec<[u32::ULE]> to have human readable serializations that show the actual integer (as opposed to a byte buffer).

This means that u32 and i32 cannot map to the same ULE type since the ULE type must have different ser/de impls. This PR adds a SignedPlainOldULE type whose only difference is in how it ser/des.

Needed by @echeran for #1353, though in his PR he probably should have a wrapper type that serializes as a Script.

@Manishearth Manishearth requested a review from sffc as a code owner December 3, 2021 05:13
@sffc
Copy link
Member

sffc commented Dec 3, 2021

I'm not totally convinced. PlainOldULE is used as the AsULE::ULE for other types in ICU4X; it's there so people can use it. If we make a special case for signed integers, it raises questions about what to do in other cases where type ULE = PlainOldULE.

@Manishearth
Copy link
Member Author

@sffc Yes, I think those questions should be raised: the answer (in this model) is to use POU only if you either:

  • don't plan to use it in VZV<[T]>
  • don't care what the human readable serialization looks like (in VZV<[T]>)
  • want the human readable serialization to work this way

otherwise you should wrap POU with your own wrapper that forwards the ULE impl.


An alternate solution is to provide ZeroVecBorrowed<T>, and move the VarULE impl on [T] to ZVBorrowed instead. This does feel cleaner to me.

@Manishearth
Copy link
Member Author

Actually, not ZVBorrowed, we need ZVULE<T>

@Manishearth
Copy link
Member Author

I can try implementing that instead, I think it would be pretty clean.

@Manishearth
Copy link
Member Author

#1357. I definitely like this better

@Manishearth
Copy link
Member Author

Closing in favor of #1357 , it seems like @echeran has also started using this and it turns out nicely

@Manishearth Manishearth closed this Dec 4, 2021
@Manishearth Manishearth deleted the ule-serialize branch February 10, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants