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
Deprecate unwrapped and implement unwrap and unwrapping #2741
Conversation
@hkaiser did you take a look on this already? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very nice, overall.
using element_of_t = typename std::conditional< | ||
std::is_rvalue_reference<Container&&>::value, | ||
decltype(std::move(*(std::declval<Container>().begin()))), | ||
decltype(*(std::declval<Container>().begin()))>::type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be simplified (i.e. encapsulated in a separate facility)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
element_of_t
also emulates the behaviour of container_accessor
which means that it's quite difficult to refactor this into a seperate facility. However, I'm open for suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just find this construct to be difficult to parse. Could you at least add an explaining comment, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more documentation to this alias.
{ | ||
} | ||
|
||
auto begin() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll ckeck that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The begin
and end
method can't be a const references because we must allow the mapping from non const l-value references too (this was fixed today).
return std::make_move_iterator(container_.begin()); | ||
} | ||
|
||
auto end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@@ -580,6 +644,13 @@ namespace util { | |||
std::forward<T>(element)); | |||
} | |||
|
|||
/// Boxes the given values into an according tuple | |||
template <typename... T> | |||
auto box(T&&... args) -> tuple<T...> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale to use auto
for return type deduction here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did it for consistency, I'll replace it.
template <typename... T> | ||
auto box(T&&... args) -> tuple<T...> | ||
{ | ||
return tuple<T...>{std::forward<T>(args)...}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be make_tuple
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, The use of tuple<T...>{ ... }
is the result of a earlier compilation issue with preserving l-value references across mapping, for instance this example would fail:
// Multiple args: Make it possible to pass move only objects in
// as reference, while returning those as reference.
{
std::unique_ptr<int> ptr1(new int(6));
std::unique_ptr<int> ptr2(new int(7));
hpx::util::tuple<std::unique_ptr<int> const&,
std::unique_ptr<int> const&>
ref = map_pack(
[](std::unique_ptr<int> const& ref)
-> std::unique_ptr<int> const& {
// ...
return ref;
},
ptr1, ptr2);
HPX_TEST_EQ((*get<0>(ref)), 6);
HPX_TEST_EQ((*get<1>(ref)), 7);
*ptr1 = 1;
*ptr2 = 2;
HPX_TEST_EQ((*get<0>(ref)), 1);
HPX_TEST_EQ((*get<1>(ref)), 2);
}
Because make_tuple
comverts l-value references to actual copies of the objects, while forward_as_tuple
doesn't take ownership of r-values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood.
hpx/util/detail/unwrap_impl.hpp
Outdated
/// configuration. | ||
/// | ||
template <bool AllowsVoidFutures, bool UnwrapTopLevelTuples> | ||
struct unwrap_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be visible to the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is an implementation detail which can be removed as soon as the old unwrapped
function was completely removed from the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to introduce a preprocessor compatibility constant right away and use it to wrap this code.
Our policy for introducing breaking changes is to do it in 3 stages (releases):
- Wrap the old API in a preprocessor macro, but leave it ON by default. Also mark the old API as deprecated (at least in the docs)
- Change the macro to be OFF by default, allowing for the user to still revert to the old API
- Remove the old API altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint. How is the naming convention for such s deprecating macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hkaiser Ah the actual issue here is that the code is used from both versions: the new and the old one. Thus I decided to use compile-time value based configuration here intead of preprocessor macros.
However, this way makes it difficult to remove the real old unwrapped behaviour since it will be still available in the dead paths of the configuration then.
Maybe it would be better here, to just duplicate the file into two pieces, where one can be removed when the old unwrapped is removed (this is also better for the compile time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'll change this then as suggested as soon as I know which capabilties end up into the final version of the new unwrapped.
/// - `hpx::future<int>` -> `int` | ||
/// - `hpx::future<std::vector<float>>` -> `std::vector<float>` | ||
/// - `std::vector<future<float>>` -> `std::vector<float>` | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this unwrap std::vector<std::vector<hpx::future<T>>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would. Also futures nested inside arbitrary combinations of tuples, pairs and homogeneous containers like std::list
or std::vector
are being remapped. For instance:
std::list<
std::pair<
int,
std::tuple<
int,
std::vector<
hpx::lcos::future<int>
>,
double
>
>
>
is also unwrapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great. We just ran into the need for when_all()
to be able to handle a vector<vector<future<T>>>
the other day...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly the underlying API is only capable of doing a synchronous unwrap which isn't suitable for the current implementation of when_all
.
In order to improve when_all
we would have to use a stackful coroutine with this API or
implement another version of the traversal API which is completely stateless (to emulate a stackless coroutine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, forgot about that. Thanks.
@Naios This is impressive work! The only thing I'd ask is for you to use meaningful commit messages. Otherwise it will be impossible to reconstruct the changes you applied, if necessary. You might even want to go back and to merge commits or to rewrite some of the history on this branch to make things more transparent. |
@hkaiser Thank you. Yes, I'll interactively rebase the commits back into the 7 first commits, so everything looks like atomic changes, as soon as I'll have a proper state. Usually I keep the commits rebased with the current master. |
a2e97b1
to
f75bb35
Compare
@hkaiser For me it seems like the functional part of this PR is done now, since all the unit tests pass with the new unwrap implementation and the changes I applied recently. Additionally, I have one outstanding question regarding the originally planned compatibility layer between Since the 1:n mapping improvements, the original int add(int c1, int c2)
{
return c1 + c2;
}
// ...
hpx::util::tuple<future<int>, future<int> > tuple =
hpx::util::forward_as_tuple(
hpx::make_ready_future(42), hpx::make_ready_future(42));
HPX_TEST_EQ(unwrapped(&add)(tuple), 42 + 42); Because Do you think this is arguable? |
That is fine. We need to add a note about the breaking change in the docs, though (here). Also, we may be able not to rename the existing |
Internally I'll rename the function calls to Because of the issues: #1404, #2456 and #1400 will be resolved, #1126 is only solved half because |
@hkaiser From my side this PR is finished and ready for merging 🎉 |
@Naios: the only real question I have is whether we should retain the old name (at least for the functional part of |
@hkaiser My personal opinion on this is to also take a new name for the functional part since the overall capability of the function was changed and thus library users should be forced to review this change. But if it's decided to keep the old name for the functional implementation, I could change it back easily. |
@Naios, Congratulations on your first pull request! After this is merged we can coordinate to get you an HPX T-shirt ;) |
@Naios: well, we have to think about code out there using
I personally think we should deprecate today's For this reason, may I ask you to add a preprocessor constant (for instance HPX_WITH_UNWRAPPED_COMPATIBILITY |
@hkaiser I applied the requested changes as discussed. |
hpx/util/unwrapped.hpp
Outdated
// "hpx::util::unwrap and hpx::util::unwrapping " | ||
// "and might be removed in a later version of HPX!") | ||
// #endif // HPX_NO_DEPRECATE_UNWRAPPED | ||
#if !defined(HPX_NO_DEPRECATE_UNWRAPPED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this preprocessor constant come from? Do we really need this? I think the deprecation message should be shown unconditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hkaiser This preprocessor constant is required for the unwrapped.cpp
unit test, which still uses the old implementation for testing. Since I didn't want to yield deprecation messages from the said unit test, I introduced the conditional deprecation message.
As alternative I could remove the old unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...or simply let the test generate the deprecate warning...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better not to generate any warnings so we are still able to build the tests using -Werror
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's then rather remove the unit test for a feature which will be removed anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hkaiser I removed the unit test so this point should be resolved
* Partially re-uses the previous tests of tests/unit/util/unwrapped.cpp
* Correct the naming of some functions
* Prevents wrong function selection caused through ADL * Also namespace a call to make_tuple
* Use hpx::util::unwrap and hpx::util::unwrapping instead, that clearify which underlying implementation should be used (the immediate or the deferred one). The automatic implementation selection was broken since unwrapped allowed to pass non future arguments through. * Closes STEllAR-GROUP#1400 * Closes STEllAR-GROUP#1404 * Closes STEllAR-GROUP#2456 * Ref STEllAR-GROUP#1126 * Ref STEllAR-GROUP#1132
There was a problem hiding this 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!
@Naios What compilers this PR was tested with?
There is the error message above. (Sorry to korean error message. Maybe you need to translate the message to english.) |
@taeguk I'm sorry because it seems the PR was only tested with with Visual Studio 2017. What you could try is to patch your locale Visual Studio version to Visual Studio 2015 Update 3, which eventually could fix your issue. @hkaiser Is the current minimum compiler version for HPX still Visual Studio 2015 as described in the docs or could it be raised to Visual Studio 2017? |
So far we've had the policy to support the latest two versions of Visual Studio. We just dropped support for VS 2013 the other day,
Ok, patches welcome. Anybody?
That should be doable as well. |
@Naios: another option would be to leave the old unwrapped code in place when compiling with VS2015. I don't particularly care about this version as VS2017 is ABI compatible and everybody can simply download the newer one. This however would require some discussion, I'd like to hear the opinion of others. |
* 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
* 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
@Naios I found that issue with Visual Studio 2015 Enterprise Update 3. |
* 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
@taeguk I'm really sorry for the inconveniences. As temporary workaround you may cherry-pick the fix locally: git remote add msvcfix https://github.com/Naios/hpx.git
git fetch msvcfix
git cherry-pick 320f515335b43f0342 Or as alternative you could exclude this PR from your locale history through interactive rebasing |
@Naios Thanks for your fast fix! |
* 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
* 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
This is the second PR of my GSoC 🌞 project
unwrapping
, the new immediate unwrap:unwrap
. The API provides two alternatives for both functions:unwrap_n
,unwrap_all
,unwrapping_n
,unwrapping_all
which unwrap until a particular level of futures or all futures which are found recursively inside the arguments.unwrap
aren't unwrapped anymore. We could provide anexplode
orspread
function for this behaviour.unwrapped
which calls the new APIunwrap
.Probably there are some remaing build failures and inconsisties which need to be fixed, however I want to receive feedback early.
For now:
✔️
hpx
base library builds✔️
unwrapped_test
builds (using the unit tests of the old implementation for the new one)✔️
unwrap_test
builds✔️
examples
build✔️
tests
buildOutstanding points (need to be fixed before merging):
unwrap
andunwrapping
using namespace
constexpr
andnoexcept
unwrapped
byunwrap
andunwrapping