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

WIN32 is not necessarily defined _WIN32 should be #24

Closed
wants to merge 1 commit into from
Closed

Conversation

agravgaard
Copy link
Contributor

@agravgaard agravgaard commented Apr 21, 2017

Typo or intentional?
_WIN32 is described in https://msdn.microsoft.com/en-us/library/b0084kay(v=vs.140).aspx for all editions WIN32 is not defined for any. Thus the reason to not change this, would be to somehow take into account windows c++ compilers that does not define this flag by default. However, on my combination of cmake flags WIN32 were not defined at compile-time.
(On Windows 8.1 with Visual studio v140, v120 and the Intel C++ Compiler 17.0 - Using ExternalProject_Add almost the same way Slicer do, but without the VTK dependency)

Typo or intentional? 
_WIN32 is described in https://msdn.microsoft.com/en-us/library/b0084kay(v=vs.140).aspx for all editions WIN32 is not defined for any. Thus the reason to not change this would be to somehow take into account windows c++ compilers that does not define this flag by default. However, on my combination of cmake flags WIN32 were not defined at compile-time. 
(On Windows 8.1 with Visual studio v140, v120 and the Intel C++ Compiler 17.0 - Using ExternalProject_Add almost the same way Slicer do, but without the VTK dependency)
@agravgaard agravgaard closed this Apr 24, 2017
@agravgaard agravgaard deleted the patch-1 branch April 24, 2017 15:37
@agravgaard
Copy link
Contributor Author

I realized WIN32 not being defined was due to my CMake flags not being parsed properly.

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.

1 participant