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 Pod bounds on traits #551

Closed
wants to merge 26 commits into from
Closed

Conversation

tamird
Copy link
Contributor

@tamird tamird commented May 25, 2023

Because Pod has a 'static bound, its present on many traits means it is
impossible to implement those traits on any type that has a non-static
lifetime. Removing the Pod bounds requires granular bounds elsewhere, which are
added here.

@bjorn3
Copy link
Contributor

bjorn3 commented May 25, 2023

An 'static bound on a type means that values of this type can life forever. Not that they actually do. In practice this means that any references contained inside this type are 'static. Pod types can't contain references so the 'static bound doesn't restrict anything.

@bjorn3
Copy link
Contributor

bjorn3 commented May 25, 2023

To elaborate on why Pod types can't contain references, the safety condition of Pod requires that "A type that is Pod must have no invalid byte values". References have many invalid byte values, null pointers being the simplest of them. Enums also violate this safety condition. Any discriminant value other than those of a valid variant is an invalid byte value.

@tamird
Copy link
Contributor Author

tamird commented May 25, 2023

So the alternative is to remove the Pod bound from various traits e.g. FileHeader. In that case you'd move the Pod bound to methods like Parse which effectively cast a byte slice into a FileHeader. Would that be better?

@bjorn3
Copy link
Contributor

bjorn3 commented May 25, 2023

I think moving the Pod bound makes sense.

@tamird tamird changed the title Remove Pod: 'static bound Remove Pod bounds on traits May 25, 2023
@tamird
Copy link
Contributor Author

tamird commented May 25, 2023

After much toil: done.

@philipc
Copy link
Contributor

philipc commented May 25, 2023

After much toil: done.

I wish you had waited for more discussion before doing this much work. My initial reaction is that we don't want this change.

Can you show what this allows you to accomplish? I don't understand how you can implement FileHeader for an enum even with this change, or why you would want to. FileHeader probably should have been a sealed trait.

@philipc
Copy link
Contributor

philipc commented May 26, 2023

I'll discuss further in #549

@tamird
Copy link
Contributor Author

tamird commented May 26, 2023

Separately from the discussion in #549 I think it might be good to land this from a separation-of-concerns perspective. These traits don't care about being Pod apart from when they provide ingress from a byte slice. Let me know if this resonates with you.

@philipc
Copy link
Contributor

philipc commented May 27, 2023

Yes, technically the Pod bound could be moved to the parse, but in practice this trait should only be implemented for Pod types anyway, and the loss of ergonomics is not worth the technical purity. The Debug bound is similar. It shouldn't be needed at all, but it was added to make the types much more pleasant to work with.

The amount of boilerplate that you had to add in readobj indicates to me that moving the Pod bound is not the right choice.

@philipc philipc closed this Jun 6, 2023
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.

3 participants