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.

Sign up for GitHub

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

Jump to bottom

Define rust::isize with Windows support #97

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Define rust::isize with Windows support #97

merged 2 commits into from
Apr 10, 2020

Conversation

dtolnay
Copy link
Owner

@dtolnay dtolnay commented Apr 10, 2020 •
edited
Loading

Hybrid of 15b5b10 and ef1bf18 and #96 (comment).

cc @myronahn @AlesTsurko
closes #96

@dtolnay
Copy link
Owner Author

dtolnay commented Apr 10, 2020 •
edited
Loading

Following up on _WIN32 vs _MSC_VER from #96 (comment): it sounds like _WIN32 is defined when building for windows with any compiler, while _MSC_VER is defined when building specifically with the Microsoft compiler. Unclear which is more appropriate in this case but I would think _WIN32 because building the same code with a different compiler on windows isn't going to cause a different set of headers to be included (is it?). @myronahn @AlesTsurko I would appreciate any insight on this.

@ales-tsurko
Copy link

ales-tsurko commented Apr 10, 2020 •
edited
Loading

Really can't say anything reasonable about it. I'm not an expert in the Windows development.

Anyway, if this is true:

_WIN32 is defined when building for windows with any compiler, while _MSV_VER is defined when building specifically with the Microsoft compiler.

the _WIN32 is the correct choice.

I used the Visual Studio's/Microsoft compiler to check #96

@myronahn
Copy link
Contributor

I believe BaseTsd.h is in the Windows SDK and so theoretically independent of the Microsoft Compiler. _WIN32 is probably more appropriate in this case.

dtolnay
dtolnay commented Apr 10, 2020
View reviewed changes
Copy link
Owner Author

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks all, I'll go ahead with this approach then. It looks like both our x86_64-pc-windows-msvc and x86_64-pc-windows-gnu CI builds are happy with this which is a good sign. Please let me know if this solution turns out to be problematic down the line.

@dtolnay dtolnay merged commit 59b7c12 into master Apr 10, 2020
@dtolnay dtolnay deleted the isize branch April 10, 2020 19:04
@dtolnay dtolnay mentioned this pull request Apr 10, 2020
Closed
@dtolnay dtolnay mentioned this pull request Sep 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dtolnay @ales-tsurko @myronahn