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

Use generic io trait implementations for Cursor when possible #23364

Closed
wants to merge 2 commits into from

Conversation

nwin
Copy link
Contributor

@nwin nwin commented Mar 14, 2015

This PR provides generic implementations of most io traits for Cursor. The former behavior is unchanged while it is now possible to use boxed types like Box<[u8]> or future Rc<[T]>.

A generic writer implementation does not seem to be possible, because it would conflict with the desirable specialization for Vec<u8>.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@nwin
Copy link
Contributor Author

nwin commented Mar 14, 2015

It would be easy to add an implementation of Write for Box<[u8]> as well but I’m unsure how useful that would be.

@petrochenkov
Copy link
Contributor

Why Deref is used?
There's a whole bunch of traits allowing to borrow the slice from some owned container - Borrow, AsSlice, Deref - what are the conventions here?
I also suppose it will conflict with generic impl of Cursor for fixed-sized arrays here

@nwin
Copy link
Contributor Author

nwin commented Mar 14, 2015

Good questions. I chose Deref since it is more fundamental (available in core).

@alexcrichton
Copy link
Member

I personally do find it much nicer for these to be generic implementations rather than hard-coded, but I also fear that we don't have the right trait to operate generically with at this time. I feel that Deref here is a bit too heavy-handed for this purpose. To me the Deref trait should be used primarily for . syntax and not necessarily for generic programming, but I know that others may differ in opinions about this :)

A concrete backwards-compatibility risk is that this would enable something like Cursor<MutexGuard<[u8]>> where if we had a "more proper" trait later the MutexGuard type would probably not implement the trait.

cc @aturon

@nwin
Copy link
Contributor Author

nwin commented Mar 15, 2015

The alternative to Deref would be Borrow and I must admit I’m not exactly sure what the difference between these two traits is. They seem to be a bit redundant.

The lack of generic implementation is also an issue I have with the stdlib in general at the moment. I’m fearing that it will get messy when using a lot of third party provided types. If the author forgets to implement a trait you need, you end up having a lot of wrapper types to implement these traits yourself.

In the case of reference-like types, the most natural trait to implement is Deref and it is safe to assume that any third-party provided reference-type will implement Deref.

@alexcrichton
Copy link
Member

Borrow exists as the opposite of Deref because you can only dereference to one type but you can possibly be borrowed to many types. Given that this is generic over Deref, though, I'm not sure that this will actually cause too many problems in downstream libraries. For example instead of Cursor::new(mutex_guard) you would instead write Cursor::new(&*mutex_guard) which is to say that if you can Deref to [u8] then you can already get the slice out somehow for reading/writing.

@petrochenkov
Copy link
Contributor

[T; N] doesn't (and shouldn't) implement Deref<Target=[T]> but we certainly can get a slice out of it, so Deref is not general enough. On the other hand, it would be quite logical to implement Borrow<[T]> for [T; N] (it is strange that it is not implemented yet) and use Borrow<[T]> as a conventional bound for "slice-like things". Another even more general variant is to use generic conversion traits from @aturon 's RFC (something like As<&[T]>) for this purpose.

@aturon
Copy link
Member

aturon commented Mar 16, 2015

@alexcrichton

I tend to agree that we may want to take things a bit slow here. Note, however, that it's not exactly back-compat to add such an impl later, since it could introduce coherence conflicts with downstream crates. (It might be possible to do if we introduced a new trait at the same time.)

More broadly, we've had proposals like this come up for using Deref or conversion traits to make an impl more flexible/general/ergonomic. Maybe it would be a good idea to establish an explicit convention around this?

@alexcrichton
Copy link
Member

Maybe it would be a good idea to establish an explicit convention around this?

I agree! This is also somewhat covered by rust-lang/rfcs#279

@bors
Copy link
Contributor

bors commented Mar 24, 2015

☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

@nwin
Copy link
Contributor Author

nwin commented Apr 5, 2015

Should I pursue this PR but using AsRef instead? AsRef is currently also lacking implementation by Box, Rc and completely redundant to Borrow.

@alexcrichton
Copy link
Member

@nwin out of curiosity, do you have a use case for wanting to create a Cursor<Box<[u8]>> or something similar? As-is I can definitely see where this is adding more functionality, but I'm not quite sure where it actually comes up in practice.

@nwin
Copy link
Contributor Author

nwin commented Apr 6, 2015

My use case is a chat (IRC) server where the messages need to get distributed to many clients. For this I planned to use Cursor<Arc<[u8]>> in order to have a separate buffer for every client without needing to clone the message like hundred times.

@alexcrichton
Copy link
Member

Hm currently Arc<[u8]> doesn't actually work for various other reasons (T needs to be sized). I think that there's also a missing of AsRef<T> on Arc<T>, so it may not work quite yet as well :(

@alexcrichton
Copy link
Member

Hm ok this seems to have fallen by the wayside a bit, so I'm going to close this out of inactivity for now. I don't think that the use case for this generalization is super compelling today, but it doesn't mean it won't be one day! I believe the modifications here should be fine backwards-compatible additions, so the door is still open for this.

@nwin
Copy link
Contributor Author

nwin commented Jul 21, 2015

I updated my implementation. Github does not seem to recognize it if that (maybe that happens if the PR gets reopened). It now uses AsRef which means that fixed-sized array can now be used with Cursor. Besides that, the generic implementation simplifies the code as the macro can be avoided.

The only drawback is, that specialized implementation for fixed-sized arrays are now ruled out as long as RFC#1210 it not accepted & implemented.

Box<[u8]> cannot be used yet, but that should be mitigated by implementing AsRef for Box and friends. I would like to submit a separate PR for at some point (RFC should not be needed as it is a minor change according to RFC #1105). That is a separate issue though.

@alexcrichton
Copy link
Member

Yes I think that submitting a separate PR is fine to do at this point.

bors added a commit that referenced this pull request Oct 8, 2015
This is a revival of #23364. Github didn’t recognize my updated branch there.

The cursor implementation now uses `AsRef` which means that fixed-sized array can now be used with `Cursor`. Besides that, the generic implementation simplifies the code as the macro can be avoided.

The only drawback is, that specialized implementation for fixed-sized arrays are now ruled out unless [RFC#1210](rust-lang/rfcs#1210) is accepted & implemented.

`Box<[u8]>` cannot be used yet, but that should be mitigated by [implementing `AsRef` for `Box` and friends](https://internals.rust-lang.org/t/forward-implement-traits-on-smart-pointers-make-smart-pointers-more-transparent/2380/3). I will submit a separate PR for that later as it is an orthogonal issue.
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.

7 participants