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

Unwrap hotfixes #2787

Merged
merged 2 commits into from Jul 31, 2017
Merged

Unwrap hotfixes #2787

merged 2 commits into from Jul 31, 2017

Conversation

Naios
Copy link
Contributor

@Naios Naios commented Jul 29, 2017

This PR provides two hotfixes for the merged PR #2741:

  • Build fix for MSVC 2015
  • Fix unconditional moves in tuple_cat

It would be great if @K-ballo could review 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! Nice fix!

template <typename... T>
tuple<T...> create_raw_tuple(T&&... args)
{
return tuple<T...>{std::forward<T>(args)...};
Copy link
Member

Choose a reason for hiding this comment

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

Use of list-init here makes me uneasy, but since no conversions ought to be involved it should be fine.

/// A helper function for creating a non owning tuple from the given
/// arguments. Which means that the funcitonality of this function
/// lies between make_tuple and forward_as_tuple.
template <typename... T>
Copy link
Member

Choose a reason for hiding this comment

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

Prefer a name that reflects plurality of typenames, like Ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll change that

@@ -856,16 +833,16 @@ namespace hpx { namespace util
detail::pack_c<std::size_t, Is...>, detail::pack<Tuples...>
>
{
typedef tuple<
typename tuple_cat_element<Is, detail::pack<Tuples...> >::type...
> type;
Copy link
Member

Choose a reason for hiding this comment

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

With this last bit gone, there doesn't seem to be a reason anymore to keep tuple_cat_result_impl as a helper struct rather than a helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, I'll simplify this.

@@ -371,6 +371,21 @@ void tuple_cat_test()
HPX_TEST_EQ((*hpx::util::get<1>(result)), 1);
HPX_TEST_EQ((*hpx::util::get<2>(result)), 2);
}

// Don't move references unconditionally
Copy link
Member

Choose a reason for hiding this comment

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

This comment is misleading, the test below doesn't contain any checks for moves being performed or not performed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is a little bit unclear, however it checks the intended behaviour through the result data type hpx::util::tuple<int&, int&&>.

@Naios
Copy link
Contributor Author

Naios commented Jul 30, 2017

@hkaiser I applied the improvements suggested by K-ballo and fixed the remaining errors in one of the tests.

@Naios
Copy link
Contributor Author

Naios commented Jul 30, 2017

Seems like the CircleCI test failed for this because of the 120 min timeout.

* Thanks to K-ballo for pointing this out
* We use hpx::util::get now for retrieving a specific
  element at index i instead of the internal
  tuple_element API.
* Ref STEllAR-GROUP#2741
@hkaiser hkaiser merged commit bff4854 into STEllAR-GROUP:master Jul 31, 2017
@hkaiser
Copy link
Member

hkaiser commented Aug 1, 2017

@Naios
Copy link
Contributor Author

Naios commented Aug 1, 2017

@hkaiser Thank you for noticing, yes I'll have a look.

@Naios
Copy link
Contributor Author

Naios commented Aug 2, 2017

@hkaiser The unit test only fails on GCC, on Clang and MSVC everything is fine.

However I was able to reproduce the issue through following unit test on all toolchains:

    // Don't move references unconditionally (move only types)
    {
        std::unique_ptr<int> i1(new int(11));
        std::unique_ptr<int> i2(new int(22));

        hpx::util::tuple<std::unique_ptr<int>&> f1 =
            hpx::util::forward_as_tuple(i1);
        hpx::util::tuple<std::unique_ptr<int>&&> f2 =
            hpx::util::forward_as_tuple(std::move(i2));

        hpx::util::tuple<std::unique_ptr<int>&, std::unique_ptr<int>&&> result =
            hpx::util::tuple_cat(std::move(f1), std::move(f2));

        HPX_TEST_EQ((*hpx::util::get<0>(result)), 11);
        HPX_TEST_EQ((*hpx::util::get<1>(result)), 22);
    }

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