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

Supporting -std=c++17 flag #2625

Merged
merged 8 commits into from Jun 10, 2017

Conversation

Projects
None yet
4 participants
@AntonBikineev
Copy link
Contributor

commented May 13, 2017

No description provided.

AntonBikineev added some commits May 13, 2017

@Naios

Naios approved these changes May 13, 2017

Copy link
Contributor

left a comment

LGTM 👍

add_definitions(-DBOOST_NO_CXX14_CONSTEXPR)
endif()
if(HPX_WITH_CXX17)
set(CXX_FLAG -std=c++17)

This comment has been minimized.

Copy link
@Naios

Naios May 13, 2017

Contributor

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.

This comment has been minimized.

Copy link
@AntonBikineev

AntonBikineev May 13, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 13, 2017

Member

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

This comment has been minimized.

Copy link
@Naios

Naios May 13, 2017

Contributor

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?

This comment has been minimized.

Copy link
@AntonBikineev

AntonBikineev May 13, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 13, 2017

Member

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.

This comment has been minimized.

Copy link
@AntonBikineev

AntonBikineev May 13, 2017

Author Contributor

@hkaiser do you suggest to use boost::random_device here?

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 13, 2017

Member

Well, since your test for HPX_HAVE_CXX11_STD_SHUFFLE includes std::random_device we should be fine as it is.

if(HPX_WITH_CXX0X)
set(CXX_FLAG -std=c++0x)
endif()
endif()
endif()
endif()
endif()

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 13, 2017

Member

I'd suggest moving the whole -std=XXX business into a separate cmake file to avoid cluttering the main one.

@hkaiser
Copy link
Member

left a comment

See my other comments. Thanks for looking into this!

@hkaiser

hkaiser approved these changes Jun 7, 2017

@hkaiser hkaiser force-pushed the cmaks_cxx17_support branch from 5336ff6 to 9274768 Jun 8, 2017

hkaiser added some commits Jun 8, 2017

Add diagnostics printing C++ mode used
- disable C++14 and up for Intel < V17
@hkaiser

This comment has been minimized.

Copy link
Member

commented Jun 10, 2017

@AntonBikineev I have changed this PR since it was approved. Could you have another look, please?

@AntonBikineev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2017

@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...)

@hkaiser hkaiser merged commit 7d4077d into master Jun 10, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@hkaiser hkaiser deleted the cmaks_cxx17_support branch Jun 10, 2017

@msimberg msimberg added this to the 1.1.0 milestone Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.