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

Make Vec yokeable #4789

Open
ChayimFriedman2 opened this issue Apr 10, 2024 · 3 comments
Open

Make Vec yokeable #4789

ChayimFriedman2 opened this issue Apr 10, 2024 · 3 comments
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake

Comments

@ChayimFriedman2
Copy link

Yokeable is implemented for Vec, but it does not forward the yoking and instead just treat them as owned values that do not borrow from the cart. This is inconsistent with the implementation of Yokeable for arrays, which does forward to the items.

I don't see any inherent reason why Vec couldn't do the same. This is a breaking change, but with the current situation it is impossible (safely) to use Vec as a yokeable, while if we change it it is easily possible to restore the old behavior by deriving Yokeable on non-yoked structs.

@sffc
Copy link
Member

sffc commented Apr 12, 2024

It seems intuitive that the items of a Vec should be able to borrow from the cart if they are not currently able to do so.

@Manishearth

@sffc sffc added the C-zerovec Component: Yoke, ZeroVec, DataBake label Apr 12, 2024
@Manishearth
Copy link
Member

This would be acceptable, I think.

@Manishearth
Copy link
Member

Manishearth commented Apr 13, 2024

Though I think the reason we have that impl is because doing such impls on third-party crates can be annoying. But we don't care too much about Vec in ICU4X so we're not relying on it anyway. I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake
Projects
None yet
Development

No branches or pull requests

3 participants