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

Add std::io::{Read,Write} wrappers #162

Merged
merged 7 commits into from Nov 16, 2018

Conversation

Projects
None yet
4 participants
@fkrull
Copy link
Contributor

fkrull commented Oct 23, 2018

See #160. I've only done the Read impl for now because figuring out the API was trickier than I expected, so I wanted to get some feedback first.

So it turns out you can't just implement Read for any type that implements InputStreamExt (or some other trait). I came up with a few ideas for that:

  • Implement the trait for the InputStream struct, and all the structs for derived classes. Not great because it's not generic, and only works if you have the concrete type anyway.
  • Implement Read on a helper struct and allow conversion into that helper using From.
  • Implement Read on a helper struct and add an explicit method to convert to it.

I went with the last option because converting with From and Into always seemed to unsightly type annotations, involving the concrete helper type. With a custom method, the helper type can be more anonymous.

Interestingly, the compiler required the InputStreamRead struct to be public when using the function from a different crate, but it didn't require the calling code to be able to actually see it. So now client code can call into_read and use the return value, but it can't name its concrete type. Which feels a little weird.

use std::mem;
use std::ptr;
use InputStream;
use super::error::to_std_io_result;

This comment has been minimized.

@EPashkin

EPashkin Oct 23, 2018

Member

without super:: it not works?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 23, 2018

Thanks, this part looks good for me

// Copyright 2018, The Gtk-rs Project Developers.
// See the COPYRIGHT file at the top-level directory of this distribution.
// Licensed under the MIT license, see the LICENSE file or <http://opensource.org/licenses/MIT>

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 23, 2018

Member

Please remove extra line.

use Error;
use IOErrorEnum;
use std::io;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 23, 2018

Member

Please remove extra line.

impl From<IOErrorEnum> for io::ErrorKind {
fn from(kind: IOErrorEnum) -> Self {
match kind {
IOErrorEnum::Failed => io::ErrorKind::Other,

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 23, 2018

Member

Since you return io::ErrorKind::Other a lot of times, why not grouping all error returning it?

@@ -204,9 +210,19 @@ impl<O: IsA<InputStream> + IsA<glib::Object> + Clone + 'static> InputStreamExtMa
}
}

pub struct InputStreamRead<T: InputStreamExtManual>(T);

This comment has been minimized.

@sdroege

sdroege Oct 23, 2018

Member

Might be useful to also have a function to get back to the original type, and/or implement Deref for it

This comment has been minimized.

@sdroege

sdroege Nov 16, 2018

Member

Should implement Eq and Debug here at least, based on the impls of the contained type. Can probably be derived

@fkrull fkrull changed the title Add std::io::Read wrapper for gio::InputStream Add std::io::{Read,Write} wrappers Nov 14, 2018

@fkrull

This comment has been minimized.

Copy link
Contributor Author

fkrull commented Nov 14, 2018

Ok, I added the std::io::Write part as well. If there's anything else holding up this PR, let me know.

use IOErrorEnum;
use std::io;

impl From<IOErrorEnum> for io::ErrorKind {

This comment has been minimized.

@sdroege

sdroege Nov 15, 2018

Member

This is generally very useful, thanks :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 15, 2018

Looks all good to me apart from the things already mentioned in the first comments above, which are mostly cosmetic :) Update that and we can get this in

@fkrull

This comment has been minimized.

Copy link
Contributor Author

fkrull commented Nov 15, 2018

Looks all good to me apart from the things already mentioned in the first comments above, which are mostly cosmetic :) Update that and we can get this in

Oh, I fixed those earlier, I didn't add a comment for it though. Or is there anything else I missed?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 16, 2018

Oh, I fixed those earlier, I didn't add a comment for it though. Or is there anything else I missed?

Ah I just saw that the comments are still shown by github and it didn't detect them as "resolved". Will double check later :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 16, 2018

Indeed, looks all good. @GuillaumeGomez ?

@@ -179,9 +186,30 @@ impl<O: IsA<OutputStream> + IsA<glib::Object> + Clone + 'static> OutputStreamExt
}
}

pub struct OutputStreamWrite<T: OutputStreamExt>(T);

This comment has been minimized.

@sdroege

sdroege Nov 16, 2018

Member

Should implement Eq and Debug here at least, based on the impls of the contained type. Can probably be derived

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 16, 2018

Looks good to me as well. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit ac71f2f into gtk-rs:master Nov 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 16, 2018

@fkrull can you send a new PR that implements Debug and PartialEqand Eq for the new structs :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 16, 2018

And potentially something to come back from the new struct to the contained type again

@fkrull fkrull deleted the fkrull:std-io-read-write branch Nov 16, 2018

vhdirk pushed a commit to vhdirk/gio-rs that referenced this pull request Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.