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

Fix msvc annoyances #5963

Merged
merged 8 commits into from
Jun 27, 2017
Merged

Fix msvc annoyances #5963

merged 8 commits into from
Jun 27, 2017

Conversation

adrido
Copy link
Contributor

@adrido adrido commented Jun 11, 2017

Some annoyances where introduced lately, and some did exist longer.
I hope its clear what every commit is doing.

sfan5
sfan5 reviewed Jun 11, 2017
View reviewed changes
src/threading/thread.cpp Outdated
@@ -311,7 +311,7 @@ bool Thread::bindToProcessor(unsigned int proc_number)

bool Thread::setPriority(int prio)
{
#if USE_WIN_THREADS
#ifdef _WIN32
Copy link
Member

@sfan5 sfan5 Jun 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this is absolutely not how this should be done

change the header file at the point where decision making is done
it always uses c++11 now but seems like @nerzhul forgot to change these

check whether the same change is needed for bindToProcessor

Copy link
Member

Choose a reason for hiding this comment

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

yeah i forgot and it seems mingw travis build doesn't care... :o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

soo... how should I handle it?

Copy link
Member

Choose a reason for hiding this comment

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

see whether the #if USE_WIN_THREADS conditions need to be renamed or the code bock removed in this and the other function

Copy link
Contributor Author

@adrido adrido Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should be done now.
the remaining failure seems to be a failure of travis itself

sfan5
sfan5 reviewed Jun 11, 2017
View reviewed changes
src/CMakeLists.txt
@@ -708,9 +708,12 @@ include(CheckCXXCompilerFlag)

if(MSVC)
# Visual Studio
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /std:c++11")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D WIN32_LEAN_AND_MEAN /MP")
Copy link
Member

@sfan5 sfan5 Jun 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

perhaps unsurprisingly this causes a lot of warnings on mingw

@adrido adrido force-pushed the fix-msvc-annoyances branch 3 times, most recently from edcdeb0 to 2c4851b Compare June 12, 2017 10:12
@adrido adrido mentioned this pull request Jun 16, 2017
Closed
nerzhul
nerzhul reviewed Jun 16, 2017
View reviewed changes
src/threading/thread.cpp
@@ -311,7 +311,7 @@ bool Thread::bindToProcessor(unsigned int proc_number)

bool Thread::setPriority(int prio)
{
#if USE_WIN_THREADS
#ifdef _MSC_VER
Copy link
Member

Choose a reason for hiding this comment

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

use _WIN32 instead for both cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not compatible with mingw

Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor Author

@adrido adrido Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If I remember correctly because it couldn't cast the return value of getThreadHandle() to a HANDLE * (VOID *). MSVC seems to have either a overload or an implicit cast.
EDIT:
The return value of getThreadHandle() is std::thread::native_handle_type which is on Windows (MSVC) just a typedef for a HANDLE ( https://docs.microsoft.com/de-de/cpp/standard-library/thread-class#a-namenativehandlea--threadnativehandle ). On MinGW this seems to be a pthread_t type (https://stackoverflow.com/questions/19250008/mingw-stdthread-with-windows-api )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a special case for mingw.
Btw: That functions are currently not used, wouldn't it be better to completely remove them? If they are needed some day, they does still exist in git vcs.

Copy link
Member

Choose a reason for hiding this comment

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

Looks fine to me now.

@SmallJoker SmallJoker added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Windows labels Jun 17, 2017
@adrido adrido force-pushed the fix-msvc-annoyances branch 2 times, most recently from 8ec2ed6 to 703820d Compare June 21, 2017 12:01
@adrido
Copy link
Contributor Author

adrido commented Jun 21, 2017

Rebased and added another fix that prevented msvc from building: 1284717

@nerzhul
Copy link
Member

nerzhul commented Jun 26, 2017

nice @adrido .
@sfan5 can we merge ?

@nerzhul nerzhul merged commit d7343b6 into minetest:master Jun 27, 2017
@adrido
Copy link
Contributor Author

adrido commented Jun 27, 2017

Thanks

@adrido adrido deleted the fix-msvc-annoyances branch August 19, 2017 03:00
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 11, 2019
@adrido @osjc
Fix msvc annoyances (minetest#5963)
39eeddd 
* MSVC: Fix '/std:c++11' is not a valid compiler option

* MSVC/MINGW: Define 'WIN32_LEAN_AND_MEAN' for the whole project

In some obscure cases 'Windows.h" got includet before that definition, which leaded to compilation warnings+errors

* MSVC: '/arch:SSE' is only available for x86

* MSVC: Fix float conversation

* MSVC/MINGW: use winthreads on Windows

* MSVC: 'USE_CMAKE_CONFIG' might be already definied by CMake build system

* MSVC: Use all available cpu cores for compiling

* Add missing include ctime and use std::time_t
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
@adrido @osjc
Fix msvc annoyances (minetest#5963)
7aab447 
* MSVC: Fix '/std:c++11' is not a valid compiler option

* MSVC/MINGW: Define 'WIN32_LEAN_AND_MEAN' for the whole project

In some obscure cases 'Windows.h" got includet before that definition, which leaded to compilation warnings+errors

* MSVC: '/arch:SSE' is only available for x86

* MSVC: Fix float conversation

* MSVC/MINGW: use winthreads on Windows

* MSVC: 'USE_CMAKE_CONFIG' might be already definied by CMake build system

* MSVC: Use all available cpu cores for compiling

* Add missing include ctime and use std::time_t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nerzhul nerzhul nerzhul approved these changes

@sfan5 sfan5 sfan5 approved these changes

Assignees
No one assigned
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️ Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adrido @nerzhul @sfan5 @SmallJoker