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

Attempt to simplify ZeroCopyFrom #1255

Closed
Manishearth opened this issue Nov 2, 2021 · 3 comments · Fixed by #1499
Closed

Attempt to simplify ZeroCopyFrom #1255

Manishearth opened this issue Nov 2, 2021 · 3 comments · Fixed by #1499
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt

Comments

@Manishearth
Copy link
Member

Currently ZeroCopyFrom has the following signature:

pub trait ZeroCopyFrom<C: ?Sized>: for<'a> Yokeable<'a> {
    fn zero_copy_from<'b>(cart: &'b C) -> <Self as Yokeable<'b>>::Output;
}

impl<'data> ZeroCopyFrom<Foo<'data>> for Foo<'static> { .. }

It's supposed to be implemented on 'static types.

For its original purpose of helping attach_to_foo_cart() functions, it makes a lot of sense (this is why it derives from Yokeable) but there are a couple problems:

  • Since the 'static type doesn't show up in a lot of places, you really need to use a lot of explicit typing and UFCS to make stuff compile
  • Stuff like Should we have FromVarULE? #1180 can't use it

It might be better to do this instead:

pub trait ZeroCopyFrom<'data, C: ?Sized> {
    fn zero_copy_from<'data>(cart: &'data C) -> Self;
}

impl<'a, 'b: 'a> ZeroCopyFrom<'b, Foo<'b>> for Foo<'a> { .. }

If we can make this work with all of the signatures in use. There may be some need of for<'b> ZCF<'b>.

Discussed a bit with @sffc , I plan to investigate this

@Manishearth Manishearth added C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) labels Nov 2, 2021
@Manishearth Manishearth self-assigned this Nov 2, 2021
@Manishearth
Copy link
Member Author

Manishearth commented Nov 2, 2021

Non-workign attempt: #1257

it's cleaner, but the attach functions can't be called from code because rustc isn't smart enough. I might be able to get around the requirement for that particular bound

@Manishearth Manishearth removed their assignment Nov 2, 2021
@sffc sffc added T-techdebt Type: ICU4X code health and tech debt backlog labels Nov 5, 2021
@Manishearth
Copy link
Member Author

Blocked on rust on rust-lang/rust#90638 and maybe rust-lang/rust#56556

@Manishearth
Copy link
Member Author

rust-lang/rust#90950

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
2 participants