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

Fix OSX build #2790

Merged
merged 1 commit into from Aug 1, 2017
Merged

Fix OSX build #2790

merged 1 commit into from Aug 1, 2017

Conversation

jfbastien
Copy link
Contributor

@jfbastien jfbastien commented Jul 31, 2017

Linker was complaining because of multiple definitions. The header was being included in multiple places, and they were each getting and exporting their own instance of the mutex.

This was broken in 0d9d74d

Linker was complaining because of multiple definitions. The header was being included in multiple places, and they were each getting and exporting their own instance of the mutex.
@brycelelbach brycelelbach self-requested a review July 31, 2017 21:37
@brycelelbach
Copy link
Member

In C++17 we could've just made it inline. But this looks fine, presuming that CI comes back green.

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!

@@ -36,7 +36,6 @@
#else
#include <hpx/lcos/local/spinlock.hpp>
#include <mutex>
hpx::lcos::local::spinlock gmtime_call_mutex;
Copy link
Member

Choose a reason for hiding this comment

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

In C++17 we could have just added inline here.

@hkaiser
Copy link
Member

hkaiser commented Jul 31, 2017

@brycelelbach: we try to stick to C++11 (for unguarded code), so this fix is correct.

@@ -105,7 +104,8 @@ template<class convert = do_convert_format::prepend> struct high_precision_time_
// fall back to non-thread-safe version on other platforms
std::tm local_tm;
{
std::unique_lock<hpx::lcos::local::spinlock> ul(gmtime_call_mutex);
static hpx::lcos::local::spinlock mutex;
std::unique_lock<hpx::lcos::local::spinlock> ul(mutex);
Copy link
Member

Choose a reason for hiding this comment

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

This looks fine; note that this code is surrounded in the same #ifdef/#endif block as the above diff, although the #ifdef/#endif aren't visible in this patch..

@brycelelbach
Copy link
Member

@hkaiser: I know :). Just mentioning it.

@brycelelbach
Copy link
Member

Hrm, why is the AppVeyor build failing?

@hkaiser hkaiser merged commit 9c758ec into STEllAR-GROUP:master Aug 1, 2017
@jfbastien jfbastien deleted the build-fix branch August 1, 2017 16:55
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

3 participants