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

Add a synchronous mapping API #2704

Merged
merged 5 commits into from Jun 27, 2017

Conversation

Projects
None yet
3 participants
@Naios
Contributor

Naios commented Jun 19, 2017

  • Can be used to traverse or map elements contained
    in any nested container or tuple-like type.
  • Meant for replacing the internal API of
    hpx::util::unwrapped and some functions
    which are used to wait on futures.

This is the first PR for my GSoC project, which implements the unified (synchronous) internal API
for re-implementing unwrapped as well as providing helper methods for synchronous functions
that wait on futures like wait_all.

ToDo:

  • I need to fix allocator remapping across containers, which is implemented
    but only working for trivially constructible allocators as of now.
  • We need to find a solution for defining the HPX_HAVE_EXPRESSION_SFINAE macro accordingly.
    The issue is that I can't find a particular minimalistic example which could be used to detect the
    capability of the compiler to compile the API.
    All I know is, that Visual Studio isn't capable as of now to evaluate the expression SFINAE correctly.
    This leads to a less performant implementation on Windows which traverses elements although they
    contain no element, which is effectively accepted by the mapper.

Tested Compilers:

  • Visual Studio 2015 Update 3 (without HPX_HAVE_EXPRESSION_SFINAE)
  • Visual Studio 2017 (without HPX_HAVE_EXPRESSION_SFINAE)
  • GCC 4.9 (with HPX_HAVE_EXPRESSION_SFINAE)
  • GCC 5.4 (with HPX_HAVE_EXPRESSION_SFINAE)
  • Clang 3.9 (with HPX_HAVE_EXPRESSION_SFINAE)
struct my_mapper
{
template <typename T,
typename std::enable_if<std::is_same<T, int>::value>::type* = nullptr>

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

Just out of curiosity, what's the purpose of using SFINAE here?

@sithhell

sithhell Jun 19, 2017

Member

Just out of curiosity, what's the purpose of using SFINAE here?

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

Using SFINAE over operator()(int) has the benefits that wer can reject arguments that are convertible to int too.
This makes it possible to reject float values by this mapper.

@Naios

Naios Jun 19, 2017

Contributor

Using SFINAE over operator()(int) has the benefits that wer can reject arguments that are convertible to int too.
This makes it possible to reject float values by this mapper.

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

I see, so rejecting then means to discard the input value? I would have expected a compile failure in case of a missing overload. What's the rationale here?

@sithhell

sithhell Jun 19, 2017

Member

I see, so rejecting then means to discard the input value? I would have expected a compile failure in case of a missing overload. What's the rationale here?

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

The current implementation works as following: if the mapper doesn't accept a particular type, the type isn't remapped or traversed and thus effectively routed through.

Hence, there will be no compile-time error when an overload is missing or the argument type was rejected.

One of the reasons to implement it like that is, that I looked for a convenient way of routing some types through while remapping others and this approach works well with lambdas.

@Naios

Naios Jun 19, 2017

Contributor

The current implementation works as following: if the mapper doesn't accept a particular type, the type isn't remapped or traversed and thus effectively routed through.

Hence, there will be no compile-time error when an overload is missing or the argument type was rejected.

One of the reasons to implement it like that is, that I looked for a convenient way of routing some types through while remapping others and this approach works well with lambdas.

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

Makes sense.

I am not sure if this won't lead to subtle bugs though. On the other hand, those types of bugs should be caught at compile already...

@sithhell

sithhell Jun 19, 2017

Member

Makes sense.

I am not sure if this won't lead to subtle bugs though. On the other hand, those types of bugs should be caught at compile already...

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

You may still add an accept all overload, that triggers a static_assert on instantiation to implement the behaviour that only known types are valid in a mapped or traversed pack.

@Naios

Naios Jun 19, 2017

Contributor

You may still add an accept all overload, that triggers a static_assert on instantiation to implement the behaviour that only known types are valid in a mapped or traversed pack.

@sithhell

Are you planning on moving the customization points out of the detail namespace? It would be great if those were put more prominently for others to hook in them.

Show outdated Hide outdated hpx/util/pack_traversal.hpp
namespace hpx {
namespace util {
namespace detail {

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

We should probably avoid having doxygen to generate docs for this, the detail namespace is not for public consumption anyway.

@sithhell

sithhell Jun 19, 2017

Member

We should probably avoid having doxygen to generate docs for this, the detail namespace is not for public consumption anyway.

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

This was improved through one of my previous PR's already (8f71f2a).
As of now, doxygen doesn't generate docs for symbols declared inside a detail namespace.

@Naios

Naios Jun 19, 2017

Contributor

This was improved through one of my previous PR's already (8f71f2a).
As of now, doxygen doesn't generate docs for symbols declared inside a detail namespace.

Show outdated Hide outdated hpx/util/pack_traversal.hpp
std::forward<Mapper>(mapper));
return helper.init_traverse(strategy, std::forward<T>(pack)...);
}
} // end namespace detail

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

would it make sense to move the stuff in detail into a seperate file? That way users could directly see the functions to use and not get hung up with all the implementation details.

@sithhell

sithhell Jun 19, 2017

Member

would it make sense to move the stuff in detail into a seperate file? That way users could directly see the functions to use and not get hung up with all the implementation details.

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

Yes, good idea. I'll change that.

@Naios

Naios Jun 19, 2017

Contributor

Yes, good idea. I'll change that.

Show outdated Hide outdated hpx/util/pack_traversal.hpp
/// Calls the traversal method for every element in the pack,
/// and returns a tuple containing the remapped content.
template <typename First, typename Second, typename... T>
auto init_traverse(strategy_remap_tag strategy, First&& first,

This comment has been minimized.

@sithhell

sithhell Jun 19, 2017

Member

Why is first and second spelled out here?

@sithhell

sithhell Jun 19, 2017

Member

Why is first and second spelled out here?

This comment has been minimized.

@Naios

Naios Jun 19, 2017

Contributor

GCC has issues with prioritizing the deduction of the function taking only one argument over this one that accepts two or more arguments. This causes endless recursive instantiations.
In order to fix this I spelled both arguments out here.

@Naios

Naios Jun 19, 2017

Contributor

GCC has issues with prioritizing the deduction of the function taking only one argument over this one that accepts two or more arguments. This causes endless recursive instantiations.
In order to fix this I spelled both arguments out here.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 19, 2017

Contributor

@sithhell First of all thanks for your review, I highly appreciate that.

As of now, I don't have any plans on making this API more customizable than it is, mainly due to the circumstance that this API does provide enough features to implement my project and possible future requirements are not known for me.

However, the API is really adaptable in the way that we can map or traverse arbitrary types (this isn't tied to futures like in unwrapped), so my opinion is that it's most generic as possible when taking a look at the currently known requirements.

Additionally, the API can be easily refactored and extended through adapting the container_match_tag, that is responsible for providing the special mapping for a particular type category (like container or tuple-like types).

Contributor

Naios commented Jun 19, 2017

@sithhell First of all thanks for your review, I highly appreciate that.

As of now, I don't have any plans on making this API more customizable than it is, mainly due to the circumstance that this API does provide enough features to implement my project and possible future requirements are not known for me.

However, the API is really adaptable in the way that we can map or traverse arbitrary types (this isn't tied to futures like in unwrapped), so my opinion is that it's most generic as possible when taking a look at the currently known requirements.

Additionally, the API can be easily refactored and extended through adapting the container_match_tag, that is responsible for providing the special mapping for a particular type category (like container or tuple-like types).

@sithhell

This comment has been minimized.

Show comment
Hide comment
@sithhell

sithhell Jun 19, 2017

Member

Makes sense. Are you planning on doing the unchecked TODO items within that PR or will you submit following ones? If the latter, I would like to ask you to move the missing TODOs into a separate issue so that we are able to track process.

Member

sithhell commented Jun 19, 2017

Makes sense. Are you planning on doing the unchecked TODO items within that PR or will you submit following ones? If the latter, I would like to ask you to move the missing TODOs into a separate issue so that we are able to track process.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 19, 2017

Contributor

I plan to resolve the outstanding ToDo list items before this PR gets merged (the allocator issue will be resolved by tomorrow and for the capability macro I need some help from the maintainers, since I don't know how to solve it while keeping the conformance to the current practices).

Contributor

Naios commented Jun 19, 2017

I plan to resolve the outstanding ToDo list items before this PR gets merged (the allocator issue will be resolved by tomorrow and for the capability macro I need some help from the maintainers, since I don't know how to solve it while keeping the conformance to the current practices).

@sithhell

This comment has been minimized.

Show comment
Hide comment
@sithhell

sithhell Jun 19, 2017

Member

Did you have a look at this:
https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/tests/cxx11_sfinae_expression.cpp?

This test is compiled during the CMake configuration run. If compiled successful, the define HPX_HAVE_CXX11_SFINAE_EXPRESSION should be set (https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/HPX_PerformCxxFeatureTests.cmake#L27-L28). If this test doesn't fully cover your needs, we need to think about extending it. A backtrace to what is failing might be valuable here.

Member

sithhell commented Jun 19, 2017

Did you have a look at this:
https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/tests/cxx11_sfinae_expression.cpp?

This test is compiled during the CMake configuration run. If compiled successful, the define HPX_HAVE_CXX11_SFINAE_EXPRESSION should be set (https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/HPX_PerformCxxFeatureTests.cmake#L27-L28). If this test doesn't fully cover your needs, we need to think about extending it. A backtrace to what is failing might be valuable here.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 19, 2017

Contributor

Sadly HPX_HAVE_CXX11_SFINAE_EXPRESSION passes on MSVC 2017 (or is automatically set to true).
Microsoft did a lot of work on expression SFINAE recently, however it fails to evaluate the nested expression SFINAE I used, but it is capable of evaluating simple expressions.

On stackoverflow I found an example case that still fails: https://stackoverflow.com/questions/35669239/the-c-detection-idiom-is-not-working-as-expected-in-visual-studio-2015-update however, I don't think that we can reduce the issue I have in this API to a minimalistic example. Also I can't take the example over because it's MIT licensed (stackoverflow TOS).

Probably the best way to test the capability would be to try out, whether we can compile the API with HPX_HAVE_EXPRESSION_SFINAE and if it doesn't compile, we undefine it.

Contributor

Naios commented Jun 19, 2017

Sadly HPX_HAVE_CXX11_SFINAE_EXPRESSION passes on MSVC 2017 (or is automatically set to true).
Microsoft did a lot of work on expression SFINAE recently, however it fails to evaluate the nested expression SFINAE I used, but it is capable of evaluating simple expressions.

On stackoverflow I found an example case that still fails: https://stackoverflow.com/questions/35669239/the-c-detection-idiom-is-not-working-as-expected-in-visual-studio-2015-update however, I don't think that we can reduce the issue I have in this API to a minimalistic example. Also I can't take the example over because it's MIT licensed (stackoverflow TOS).

Probably the best way to test the capability would be to try out, whether we can compile the API with HPX_HAVE_EXPRESSION_SFINAE and if it doesn't compile, we undefine it.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 19, 2017

Member

Probably the best way to test the capability would be to try out, whether we can compile the API with HPX_HAVE_EXPRESSION_SFINAE and if it doesn't compile, we undefine it.

Sure this would be a viable feature test.

Member

hkaiser commented Jun 19, 2017

Probably the best way to test the capability would be to try out, whether we can compile the API with HPX_HAVE_EXPRESSION_SFINAE and if it doesn't compile, we undefine it.

Sure this would be a viable feature test.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 20, 2017

Contributor

@hkaiser, @sithhell: Today, I tried to add a feature test like described above, however I'm facing two major issues:

  • The feature test would require dependency includes (like boost), however those depend on other feature tests for configuration. -> I could move this single feature test behind the dependency setup.
  • But the major issue is that the implementation implicitly depends on the hpx/config/defines.hpp header that is generated as a result of this feature test. I can't remove this dependency since it's effectively used in other headers. -> Maybe I could generate a second define.hpp just for this feature test

Do you have any suggestions how I could solve this?

Another really simple solution for this partuclar issue would be to just check the compiler and its version, but that violates the current principle of automatic feature tests.

Contributor

Naios commented Jun 20, 2017

@hkaiser, @sithhell: Today, I tried to add a feature test like described above, however I'm facing two major issues:

  • The feature test would require dependency includes (like boost), however those depend on other feature tests for configuration. -> I could move this single feature test behind the dependency setup.
  • But the major issue is that the implementation implicitly depends on the hpx/config/defines.hpp header that is generated as a result of this feature test. I can't remove this dependency since it's effectively used in other headers. -> Maybe I could generate a second define.hpp just for this feature test

Do you have any suggestions how I could solve this?

Another really simple solution for this partuclar issue would be to just check the compiler and its version, but that violates the current principle of automatic feature tests.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 25, 2017

Contributor

@hkaiser Thanks for noticing, I corrected this.

Also the two outstanding points are fixed now:

  • Allocators are correctly detected now and remapped through the facilities of the standard library
  • Full expression SFINAE support is detected through a special feature test, that uses my implementation. Therefore the generic possibility was added to specify feature tests which use header only parts of HPX to test the compiler for a particular capability.

From my side this PR finished and thus ready for the final review and merge.

Contributor

Naios commented Jun 25, 2017

@hkaiser Thanks for noticing, I corrected this.

Also the two outstanding points are fixed now:

  • Allocators are correctly detected now and remapped through the facilities of the standard library
  • Full expression SFINAE support is detected through a special feature test, that uses my implementation. Therefore the generic possibility was added to specify feature tests which use header only parts of HPX to test the compiler for a particular capability.

From my side this PR finished and thus ready for the final review and merge.

Show outdated Hide outdated hpx/config.hpp
#include <hpx/config/version.hpp>
#endif // HPX_CONFIG_NO_INTERNAL_LINK

This comment has been minimized.

@hkaiser

hkaiser Jun 25, 2017

Member

Wouldn't it be better if this #if/#endif went into the header file hpx/config/version.hpp, instead of wrapping the include outside of it?

@hkaiser

hkaiser Jun 25, 2017

Member

Wouldn't it be better if this #if/#endif went into the header file hpx/config/version.hpp, instead of wrapping the include outside of it?

This comment has been minimized.

@Naios

Naios Jun 26, 2017

Contributor

I'm now using HPX_NO_VERSION_CHECK instead of the custom macro which made it possible to drop the conditional inclusion completely (hpx/config.hpp wasn't touched then).

@Naios

Naios Jun 26, 2017

Contributor

I'm now using HPX_NO_VERSION_CHECK instead of the custom macro which made it possible to drop the conditional inclusion completely (hpx/config.hpp wasn't touched then).

Show outdated Hide outdated tests/unit/util/pack_traversal.cpp
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)
#include <algorithm>

This comment has been minimized.

@hkaiser

hkaiser Jun 25, 2017

Member

In my experience, for MSVC, including the HPX headers first avoids various warnings.

@hkaiser

hkaiser Jun 25, 2017

Member

In my experience, for MSVC, including the HPX headers first avoids various warnings.

This comment has been minimized.

@Naios

Naios Jun 25, 2017

Contributor

Usually I try to stick to the Google C++
style guide (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) which requests to include standard library headers first for the reason:

With the preferred ordering, if dir2/foo2.h omits any necessary includes, the build of dir/foo.cc or dir/foo_test.cc will break. Thus, this rule ensures that build breaks show up first for the people working on these files, not for innocent people in other packages.

But I will reorder the includes to keep the conformance to the existing code.

@Naios

Naios Jun 25, 2017

Contributor

Usually I try to stick to the Google C++
style guide (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) which requests to include standard library headers first for the reason:

With the preferred ordering, if dir2/foo2.h omits any necessary includes, the build of dir/foo.cc or dir/foo_test.cc will break. Thus, this rule ensures that build breaks show up first for the people working on these files, not for innocent people in other packages.

But I will reorder the includes to keep the conformance to the existing code.

Show outdated Hide outdated hpx/util/pack_traversal.hpp
#ifndef HPX_UTIL_PACK_TRAVERSAL_HPP
#define HPX_UTIL_PACK_TRAVERSAL_HPP
#include <utility>

This comment has been minimized.

@hkaiser

hkaiser Jun 25, 2017

Member

Same here, please include the HPX headers first.

@hkaiser

hkaiser Jun 25, 2017

Member

Same here, please include the HPX headers first.

Show outdated Hide outdated hpx/util/detail/pack_traversal_impl.hpp
#ifndef HPX_UTIL_DETAIL_PACK_TRAVERSAL_IMPL_HPP
#define HPX_UTIL_DETAIL_PACK_TRAVERSAL_IMPL_HPP
#include <cstddef>

This comment has been minimized.

@hkaiser

hkaiser Jun 25, 2017

Member

Here as well, please include the HPX headers first.

@hkaiser

hkaiser Jun 25, 2017

Member

Here as well, please include the HPX headers first.

Naios added some commits Jun 19, 2017

Add a synchronous mapping API
* Can be used to traverse or map elements contained
  in any nested container or tuple-like type.
* Meant for replacing the internal API of
  hpx::util::unwrapped and some functions
  which are used to wait on futures.
Make it possible to run feature tests on header only parts of HPX
* add_hpx_in_framework_config_test makes HPX and its dependencies
  available to the corresponding feature test.
Add a feature test for full expression sfinae support
* Tests whether the compiler provides full expression SFINAE
  supports so all parts of HPX can be compiled.
* Currently MSVC is the only compiler that doesn't implement it.
@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Jun 26, 2017

Contributor

@hkaiser Thank you for your feedback, I corrected all mentioned points.

Contributor

Naios commented Jun 26, 2017

@hkaiser Thank you for your feedback, I corrected all mentioned points.

@hkaiser

LGTM, thanks a lot!

@hkaiser hkaiser merged commit caf2e33 into STEllAR-GROUP:master Jun 27, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment