Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

parity uses winapi 0.3.4 #8366

Merged
merged 2 commits into from
Apr 11, 2018
Merged

parity uses winapi 0.3.4 #8366

merged 2 commits into from
Apr 11, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Apr 11, 2018

No description provided.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 11, 2018
parity/main.rs Outdated
let mut wsdata: ::winapi::winsock2::WSADATA = ::std::mem::zeroed();
::ws2_32::WSAStartup(WS_VERSION, &mut wsdata);
let mut wsdata: ::winapi::um::winsock2::WSADATA = ::std::mem::zeroed();
::winapi::um::winsock2:::WSAStartup(WS_VERSION, &mut wsdata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra : before WSAStartup.

parity/url.rs Outdated
#[cfg(windows)]
pub fn open(url: &str) {
use std::ffi::CString;
use std::ptr;
use winapi::um::shellapi::ShellExecuteA;
use winapi::um::winuser::SW_SHOWNORMAL as Normal;

unsafe {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove unsafe here now!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain me why this can be removed? ShellExecuteA is still marked as unsafe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, for some reason I thought that winapi interface is safe... I should have double checked :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't the build have failed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andresilva I think CI only runs on Linux right? So, using conditional compilation flags will never compile stuff that isn't #[cfg(target_os="linux")]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right didn't think of that 👍

parity/url.rs Outdated

unsafe {
shell::ShellExecuteA(ptr::null_mut(),
ShellExecuteA(ptr::null_mut(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually ShellExecuteA can fail, why doesn't we care about that?

https://msdn.microsoft.com/en-us/library/windows/desktop/bb762153(v=vs.85).aspx


pub use self::winapi::SW_SHOWNORMAL as Normal;
}

#[cfg(windows)]
pub fn open(url: &str) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR:
Ideally, this function should return an error if the winapi or c_str's fail!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

use winapi::um::shellapi::ShellExecuteA;
use winapi::um::winuser::SW_SHOWNORMAL as Normal;

ShellExecuteA(ptr::null_mut(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I will repeat myself again because my comment was removed in the latest change!

But, actually ShellExecuteA can fail. Why doesn't we care about that?

https://msdn.microsoft.com/en-us/library/windows/desktop/bb762153(v=vs.85).aspx

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cause we're lazy :p created an issue for that :)

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 11, 2018
@debris debris merged commit 1356d6d into master Apr 11, 2018
@debris debris deleted the parity-winapi0.3.4 branch April 11, 2018 13:22
@5chdn 5chdn added this to the 1.11 milestone Apr 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants