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

[Question] Is it intentional that you can impl Try{From,Into}Ctx multiple times for a type using different ctx types? #34

Open
kitlith opened this issue Sep 10, 2018 · 3 comments

Comments

@kitlith
Copy link

kitlith commented Sep 10, 2018

impl<'a> TryFromCtx<'a, ()> for SomeType { /* does one thing */ }

impl<'a> TryFromCtx<'a, scroll::Endian> for SomeType { /* does something completely different */ }

is a situation that is totally possible to come up, and I bet that in this sort of case the methods pread and gread would not function as it wouldn't be able to figure out which trait implementation to use.

pread_with and gread_with should still function because you can disambiguate the context by providing an argument.

However, really, this begs the question: did you intend for this to be possible in the first place? Do you still want it to be possible?

@m4b
Copy link
Owner

m4b commented Sep 11, 2018

Yes it's possible, and will have the ambiguous behavior in the case of pread as you describe. I haven't thought much about whether I want it to be possible or not; I guess not?

If you have an idea how to eliminate ambiguity that is backwards compatible, I'd be interested, but I'm not sure whether it is worth a large time investment, since I don't think this issue comes up for most people (why do they want two different contexts, and if so, they should just use the _with methods to disambiguate it)

@kitlith
Copy link
Author

kitlith commented Sep 18, 2018

I'm not sure if there's a way to do it with backwards compatibility, but I think you'd accomplish only being able to implement once by having the context type be an associated type instead of a generic parameter. Whether you can create a backwards-compatible wrapper or not is another question.

Maybe put it on a "if I'm already going to make a breaking change, think about making a breaking change here too" list?

@whatisaphone
Copy link

I think it makes sense for it to be possible. It's a nice elegant way to describe how to read the same type in different ways:

let str1 = buf.gread_with::<&str>(offset, LengthPrefixed);
let str2 = buf.gread_with::<&str>(offset, FixedLength(123));

// LengthPrefixed and FixedLength definitions left as an exercise for the reader

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

No branches or pull requests

3 participants