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

Remove AsVarULE #1126

Merged
merged 3 commits into from
Oct 4, 2021
Merged

Remove AsVarULE #1126

merged 3 commits into from
Oct 4, 2021

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 29, 2021

Fixes #1078

This gets rid of AsVarULE, getting everyone to work off of the unsized VarULE types directly.

This makes ZeroVec and VarZeroVec a bit asymmetric, ZeroVec takes AsULE types and internally stores them as ULE, performing convenient conversions at the boundary. OTOH, VarZeroVec now directly takes VarULE types, which in the case of str and [u8] are quite convenient to work with already, but less so for types like VarZeroVecULE. This is kinda fine: because ULE types can be held on the stack, ZeroVec actually can make an attempt to do basically free conversions at the boundary, OTOH VarZeroVec must use owned values to do the same thing, and this can be a little expensive.

The major changes here:

  • All VarZeroVecFoo types now take T: VarULE
  • VarZeroVec::to_vec() returns `Box
  • Deserialization of VarZeroVec requires Box<T> implement Deserialize
  • ZeroMap operates on references in more place.
  • ZeroMap::replace()/ZeroMap::remove() and ZeroMap::insert() return Box<T> for the old value

@Manishearth Manishearth requested a review from sffc as a code owner September 29, 2021 23:33
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • utils/zerovec/src/map/vecs.rs is different
  • utils/zerovec/src/ule/mod.rs is different
  • utils/zerovec/src/varzerovec/owned.rs is different
  • utils/zerovec/src/varzerovec/serde.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth
Copy link
Member Author

(Rebased over #1127)

sffc
sffc previously approved these changes Oct 4, 2021
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I really like the new looks of VarZeroVec!

utils/zerovec/src/map/serde.rs Show resolved Hide resolved
utils/zerovec/src/ule/mod.rs Show resolved Hide resolved
utils/zerovec/src/varzerovec/ule.rs Show resolved Hide resolved
utils/zerovec/src/yoke_impls.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member

sffc commented Oct 4, 2021

Request: please approve and merge #1121 before merging this one, since there may be merge conflicts, and it is probably easier to resolve them in that direction. Thank you!

@Manishearth Manishearth merged commit 8a8c0ab into unicode-org:main Oct 4, 2021
@Manishearth Manishearth deleted the asvarule branch October 4, 2021 16:37
robertbastian pushed a commit to robertbastian/icu4x that referenced this pull request Oct 4, 2021
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-commit LGTM

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.

Restructure AsVarULE or get rid of it
3 participants