Supporting -std=c++17 flag #2625
Conversation
CMakeLists.txt
Outdated
| add_definitions(-DBOOST_NO_CXX14_CONSTEXPR) | ||
| endif() | ||
| if(HPX_WITH_CXX17) | ||
| set(CXX_FLAG -std=c++17) |
There was a problem hiding this comment.
Since Visual Studio 2015 the Microsoft compiler require a switch for later standard versions (C++17+) too where C++14 is the default: /std:c++14 and /std:c++latest (see here).
A (maybe better) build system driven way of depending on compiler features is the cmake target_compile_features property, however this feature was introduced in cmake version 3.1, which isn't required by HPX as of now.
There was a problem hiding this comment.
Naios, thanks for the note. Yeah, target_compile_features would be a great independent way of handling these cmd args once we introduce support for cmake-3.1.
There was a problem hiding this comment.
We've raised the minimal version of cmake to 3.0.2 already, I wouldn't mind making 3.1 the minimal requirement.
| std::random_device random_device; | ||
| std::mt19937 generator(random_device()); | ||
| std::shuffle(libdata.begin(), libdata.end(), std::move(generator)); | ||
| #else |
There was a problem hiding this comment.
Wouldn't it be better if we provide a generic hpx::container_shuffle for encapsuling the best backend which is currently available, instead of replacing all usage cases of std::random_shuffle with conditional compilation?
There was a problem hiding this comment.
It's the only place where random-shuffle is used. That said I think that minimal versions of compilers that we support are good enough to have std shuffle in their default standard libraries. So we probably don't even need this cmake test at all. What do others think about it?
There was a problem hiding this comment.
In my experience, std::random_device is a bigger hurdle to compatibility... It's one of the reasons we normally use the boost counterpart in other places. But I have not checked recently whether the situation has changed.
There was a problem hiding this comment.
@hkaiser do you suggest to use boost::random_device here?
There was a problem hiding this comment.
Well, since your test for HPX_HAVE_CXX11_STD_SHUFFLE includes std::random_device we should be fine as it is.
CMakeLists.txt
Outdated
| if(HPX_WITH_CXX0X) | ||
| set(CXX_FLAG -std=c++0x) | ||
| endif() | ||
| endif() | ||
| endif() | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
I'd suggest moving the whole -std=XXX business into a separate cmake file to avoid cluttering the main one.
hkaiser
force-pushed
the
cmaks_cxx17_support
branch
from
5336ff6
to
9274768
Compare
- disable C++14 and up for Intel < V17
|
@AntonBikineev I have changed this PR since it was approved. Could you have another look, please? |
|
@hkaiser it looks great. Sorry, I just postponed addressing your comments. I also remember there was an example which failed to compile with -std=c++1z (it used Boost.Phoenix under Boost.Spirit and there was std::random_shuffle or something in there...) |
No description provided.