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

No sizing #291

Closed
wants to merge 2 commits into from
Closed

No sizing #291

wants to merge 2 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 9, 2018

This came out of #287. Review please @burdges?

@burdges
Copy link
Contributor

burdges commented Mar 9, 2018

I don't see anything wrong with removing the Self: Sized bounds, but I've no idea why they ever existed either.

Why did you remove a + ?Sized? It might be + ?Sized belongs in more places, like the subtrait bounds, but not sure. Can we ask Alex or someone?

I find the impl<R: ..., T: DerefMut<Target = R>> for T ... interesting but ambitious. We want impl<'a,R: Rng...> Rng... for &'a mut R ... because we need code to be polymorphic over both the base R: Rng and the &'a mut R. I think your T: DerefMut<Target = R> trick makes sense for exactly the same reasons, thus avoiding all the "deref inheritance is an anti-pattern" arguments : rust-unofficial/patterns@c2cf5e6 Ask around this perhaps though?

@dhardy
Copy link
Member Author

dhardy commented Mar 10, 2018

Some of those where Self: Sized bounds were there because originally they were required (in order to support generic methods on a trait which is otherwise object-safe); I'm not sure if the change here is due to a compiler change or due to the generic fns all being moved to an extension trait.

I didn't remove a + ?Sized anti-bound; I added it to an example.

This isn't abusing Deref for inheritance like the anti-pattern you listed; it's implementing a trait for anything Deref-able to a type implementing the trait — @nrc you could maybe even consider this a pattern; are there any catches I'm missing?

@dhardy
Copy link
Member Author

dhardy commented Mar 14, 2018

Rebased on #288. I will wait a couple more days for comment then merge.

@dhardy dhardy added the E-question Participation: opinions wanted label Mar 14, 2018
@pitdicker
Copy link
Contributor

@dhardy Have you seen rust-lang/api-guidelines#158?

@dhardy
Copy link
Member Author

dhardy commented Mar 14, 2018

Thanks for asking. The error message issue is certainly a good reason not to use the Deref-based impl.

Edit: no-sizing commit merged directly, other commit abandoned.

@dhardy dhardy closed this Mar 14, 2018
@dhardy dhardy deleted the no-sizing branch March 23, 2018 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted P-high Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants