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

Rebase Windows support onto RawHandle instead of File #31

Merged
merged 1 commit into from
Sep 19, 2021
Merged

Rebase Windows support onto RawHandle instead of File #31

merged 1 commit into from
Sep 19, 2021

Conversation

adamreichold
Copy link

This is a follow-up to #30 and allows mapping anything that implements AsRawHandle including the types of async-std.

I took the Win32 API usage from Rust's standard library and thereby adjusted the existing structure declarations to match.

I did test this under Wine using the async-std cat example as in #30 but I have to admit to not feeling particularly confident on this as Win32 is not a platform I regularly target.

@RazrFalcon
Copy link
Owner

Nice! Should I merge #30 first?

src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
@RazrFalcon
Copy link
Owner

Adding more raw winapi calls is not great, but looks like there is no other way.

@adamreichold
Copy link
Author

Nice! Should I merge #30 first?

Yes, #30 is simple and safe while this is bound to get more involved.

Adding more raw winapi calls is not great, but looks like there is no other way.

Well, I could actually use fs::File::from_raw_handle to turn the given handle back into an instance of fs::File. This way I could use basically the same API calls as now, but I am not sure if fs::File makes any extra assumptions on the handle (that the raw Win32 API calls do not make) and personally, I would say that roundabout way would be quite confusing.

A different maintainability/clean-up change I could follow-up with would be to replace the manual FFI declarations by usage of the winapi crate? But I am not sure how downstream consumers (most importantly rustc) would like that.

@RazrFalcon
Copy link
Owner

The whole reason this fork exist is to remove the winapi crate dependency, so no. The current code if fine.

This allows mapping anything that implements AsRawHandle
including the types of async-std.
@RazrFalcon RazrFalcon merged commit 0632c6a into RazrFalcon:master Sep 19, 2021
@RazrFalcon
Copy link
Owner

Thanks again for your work! Do you plan any other changes or can I publish a new release?

@adamreichold
Copy link
Author

Thanks again for your work! Do you plan any other changes or can I publish a new release?

I am currently pondering whether the assertions in the Drop impls for MmapInner should be removed in the line with the discussion at rust-lang/lang-team#97 I am currently looking into standard library types and they all seem to ignore being unable to close/free/release resources during drop, e.g. https://doc.rust-lang.org/stable/src/std/io/buffered/bufwriter.rs.html#669-674 What do you think?

@adamreichold adamreichold deleted the raw-handles branch September 19, 2021 15:38
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.

2 participants