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

Should we have FromVarULE? #1180

Closed
2 tasks done
Tracked by #1082
sffc opened this issue Oct 17, 2021 · 13 comments
Closed
2 tasks done
Tracked by #1082

Should we have FromVarULE? #1180

sffc opened this issue Oct 17, 2021 · 13 comments
Labels
C-data-infra Component: provider, datagen, fallback, adapters help wanted Issue needs an assignee S-small Size: One afternoon (small bug fix or enhancement) T-enhancement Type: Nice-to-have but not required

Comments

@sffc
Copy link
Member

sffc commented Oct 17, 2021

Part of #1082

Split from #1179

If we want to support cases where the target type borrows from the VarULE, FromVarULE can work (playground):

trait FromVarULE<'a, U: ?Sized>: 'a {
    fn from_var_ule(ule: &'a U) -> Self;
}

impl FromVarULE<'_, str> for String {
    fn from_var_ule(ule: &str) -> Self {
        ule.to_string()
    }
}

impl<'a> FromVarULE<'a, str> for Cow<'a, str> {
    fn from_var_ule(ule: &'a str) -> Self {
        Cow::Borrowed(ule)
    }
}

Thoughts? Needs input from:

@sffc sffc added T-enhancement Type: Nice-to-have but not required C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) needs-approval One or more stakeholders need to approve proposal labels Oct 17, 2021
@Manishearth
Copy link
Member

Yeah so I do want this, but my general thoughts are that this function will basically be called by users not the VZV/ZV APIs, so it's not important to have a trait for it unless we're doing a custom derive (which we should absolutely do eventually). My suggestion to Zibi for now was to implement this as an inherent method on the ULE type (.as_foo(), we have .as_varzerovec() on VZVULE for example). Happy to have a trait early so that we can document this though; it's personally low-priority for me since it can be done as an inherent method.

@sffc
Copy link
Member Author

sffc commented Oct 17, 2021

Would a method such as get_decoded be useful?

impl<'a, U> VarZeroVec<'a, U>
where
    U: VarULE + ?Sized
{
    fn get_decoded<T>(&'a self) -> Option<T>
    where
        T: FromVarULE<'a, U>
    {
        todo!()
    }
}

@Manishearth
Copy link
Member

yes! it would need an index argument, but yeah that sounds useful

DecodeFromVarULE?

We can actually merge EncodeAs and DecodeFrom but there are types that are encodable but not decodable (e.g. String can't be "decoded" as a cheap borrow)

@sffc
Copy link
Member Author

sffc commented Oct 18, 2021

Would ZeroCopyFrom do the trick, instead of FromVarULE?

@Manishearth
Copy link
Member

Yes, I think so

@Manishearth
Copy link
Member

Though I think ZCF is a bit weird bc it requires Yokeable. I'll have to think about that

@sffc
Copy link
Member Author

sffc commented Oct 18, 2021

FromVarULE is a special case of ZCF: the case when the source (the Yokeable) has no lifetime parameter.

Let's not combine encode/decode like we did with the old AsVarULE. I prefer smaller traits when possible, and I think it results in a better outcome anyway.

@Manishearth
Copy link
Member

Nah, we need the lifetime parameter I think, and we also need the Yokeable. That's somewhat unfortunate but it is what it is.

@Manishearth
Copy link
Member

When we have GATs a lot of this should simplify, but that might be a ways off

@sffc
Copy link
Member Author

sffc commented Oct 18, 2021

Discussion: add FromVarULE as its own trait for now, and see if we can unify it with ZeroCopyFrom later in #1186.

@Manishearth
Copy link
Member

Related: #1255 . If we can make that happen first then we won't need a separate trait

@Manishearth
Copy link
Member

Now that ZCF is simpler I think we should just use ZCF here. It might be worth having the proc macro autogen the impl if asked, though!

@Manishearth
Copy link
Member

I'm going to close this issue then

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 help wanted Issue needs an assignee S-small Size: One afternoon (small bug fix or enhancement) T-enhancement Type: Nice-to-have but not required
Projects
None yet
Development

No branches or pull requests

2 participants