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

Don't test inplace_merge with libc++ #3078

Merged
merged 3 commits into from Dec 25, 2017

Conversation

msimberg
Copy link
Contributor

AFAICT this is not yet in any released version of libc++ so I'm ignoring the test for all versions of libc++.

I'll update this to check the version as well once the fix has been merged.

"Fixes" #2964.

@msimberg msimberg added this to the 1.1.0 milestone Dec 15, 2017
Copy link
Member

@taeguk taeguk left a comment

Choose a reason for hiding this comment

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

How about adding check of libc++ to not source code, but cmake file?
In current, the user can misunderstand like the test for inplace_merge is success.
As I think, it is better that test_inplace_merge is not built when libc++ is used.

@@ -47,9 +47,11 @@ int hpx_main(boost::program_options::variables_map& vm)
std::cout << "using seed: " << seed << std::endl;
std::srand(seed);

#if !defined(_LIBCPP_VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, it's only a problem up until version 5.

@msimberg
Copy link
Contributor Author

Updated to allow the test with libc++ version >= 6 as it runs successully on the pycicle clang 6 builder.

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

I'd suggest moving this into cmake (while disabling the test altogether) instead of making the test pass. That also allows to keep these workarounds in one spot for later changes.

@msimberg
Copy link
Contributor Author

Hmm, I think disabling the test completely is almost as bad as having it always pass.

I've now disabled only the stability tests, as the others should still pass. But I also added a cmake test for it (HPX_HAVE_STABLE_INPLACE_MERGE). @hkaiser @taeguk What do you think about this?

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!


#if defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 6)
# error "libc++ inplace_merge implementation is broken"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking the stdlib version, couldn't we actually test if inplace_merge is actually stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that does sound like the correct thing to do. Need to make sure it's thorough enough to catch future bugs in inplace_merge as well though, so to be honest I think the current check is simple and good enough. (libc++ is unlikely to have this bug again if they've added good tests for it, but then we need not trust them of course...)

@taeguk
Copy link
Member

taeguk commented Dec 22, 2017

@msimberg LGTM, too!

Apart from this PR, the user can still use inplace_merge.
Is there need to restrict or warn for using inplace_merge with libc++ < 6 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants