Add a synchronous mapping API #2704

Naios · 2017-06-19T18:08:08Z

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)

sithhell · 2017-06-19T18:40:39Z

tests/unit/util/pack_traversal.cpp

+struct my_mapper
+{
+    template <typename T,
+        typename std::enable_if<std::is_same<T, int>::value>::type* = nullptr>


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

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.

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?

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.

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...

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.

sithhell · 2017-06-19T18:44:39Z

hpx/util/pack_traversal.hpp

+
+namespace hpx {
+namespace util {
+    namespace detail {


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

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.

sithhell · 2017-06-19T18:48:34Z

hpx/util/pack_traversal.hpp

+                std::forward<Mapper>(mapper));
+            return helper.init_traverse(strategy, std::forward<T>(pack)...);
+        }
+    }    // end namespace detail


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.

Yes, good idea. I'll change that.

sithhell · 2017-06-19T18:49:56Z

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,


Why is first and second spelled out here?

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 · 2017-06-19T19:11:23Z

@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 · 2017-06-19T21:19:00Z

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 · 2017-06-19T21:32:24Z

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 · 2017-06-19T22:26:16Z

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 · 2017-06-19T22:53:35Z

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 · 2017-06-19T23:20:52Z

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 · 2017-06-20T15:17:34Z

@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 · 2017-06-25T16:28:41Z

@Naios: inspect is still unhappy with this: https://7080-4455628-gh.circle-artifacts.com/0/tmp/circle-artifacts.B1i3Bir/hpx_inspect_report.html

Naios · 2017-06-25T21:23:58Z

@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.

hkaiser · 2017-06-25T23:12:46Z

hpx/config.hpp

 #include <hpx/config/version.hpp>
+#endif // HPX_CONFIG_NO_INTERNAL_LINK


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?

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).

hkaiser · 2017-06-25T23:20:35Z

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>


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

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.

hkaiser · 2017-06-25T23:21:14Z

hpx/util/pack_traversal.hpp

+#ifndef HPX_UTIL_PACK_TRAVERSAL_HPP
+#define HPX_UTIL_PACK_TRAVERSAL_HPP
+
+#include <utility>


Same here, please include the HPX headers first.

hkaiser · 2017-06-25T23:21:42Z

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>


Here as well, please include the HPX headers first.

* add_hpx_in_framework_config_test makes HPX and its dependencies available to the corresponding feature test.

* 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.

* 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 · 2017-06-26T12:03:49Z

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

hkaiser

LGTM, thanks a lot!

hkaiser added category: core type: enhancement labels Jun 19, 2017

hkaiser added this to the 1.1.0 milestone Jun 19, 2017

sithhell reviewed Jun 19, 2017

View reviewed changes

Naios force-pushed the pack_traversal branch from 464579f to d41263f Compare June 19, 2017 23:05

Naios force-pushed the pack_traversal branch from d41263f to 8a8cb49 Compare June 24, 2017 23:09

Naios force-pushed the pack_traversal branch 2 times, most recently from 1046beb to c8c854d Compare June 25, 2017 21:00

hkaiser reviewed Jun 25, 2017

View reviewed changes

Naios added 5 commits June 26, 2017 13:58

Fix custom include and link directories in config tests

bcf25f4

Append -D to COMPILE_DEFINITIONS passed to add_hpx_config_test

880cfb7

Make it possible to run feature tests on header only parts of HPX

7586970

* add_hpx_in_framework_config_test makes HPX and its dependencies available to the corresponding feature test.

Add a synchronous mapping API

4788bdb

* 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.

Add a feature test for full expression sfinae support

7108fff

* 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 force-pushed the pack_traversal branch from c8c854d to 7108fff Compare June 26, 2017 12:00

hkaiser approved these changes Jun 26, 2017

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a synchronous mapping API #2704

Add a synchronous mapping API #2704

Naios commented Jun 19, 2017 •

edited

sithhell Jun 19, 2017

Naios Jun 19, 2017

sithhell Jun 19, 2017

Naios Jun 19, 2017 •

edited

sithhell Jun 19, 2017

Naios Jun 19, 2017

sithhell left a comment

sithhell Jun 19, 2017

Naios Jun 19, 2017

sithhell Jun 19, 2017

Naios Jun 19, 2017

sithhell Jun 19, 2017

Naios Jun 19, 2017

Naios commented Jun 19, 2017 •

edited

sithhell commented Jun 19, 2017

Naios commented Jun 19, 2017

sithhell commented Jun 19, 2017

Naios commented Jun 19, 2017 •

edited

hkaiser commented Jun 19, 2017

Naios commented Jun 20, 2017 •

edited

hkaiser commented Jun 25, 2017

Naios commented Jun 25, 2017 •

edited

hkaiser Jun 25, 2017

Naios Jun 26, 2017

hkaiser Jun 25, 2017

Naios Jun 25, 2017

hkaiser Jun 25, 2017

hkaiser Jun 25, 2017

Naios commented Jun 26, 2017

hkaiser left a comment

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

Add a synchronous mapping API #2704

Add a synchronous mapping API #2704

Conversation

Naios commented Jun 19, 2017 • edited

ToDo:

Tested Compilers:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Jun 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sithhell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios commented Jun 19, 2017 • edited

sithhell commented Jun 19, 2017

Naios commented Jun 19, 2017

sithhell commented Jun 19, 2017

Naios commented Jun 19, 2017 • edited

hkaiser commented Jun 19, 2017

Naios commented Jun 20, 2017 • edited

hkaiser commented Jun 25, 2017

Naios commented Jun 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios commented Jun 26, 2017

hkaiser left a comment

Choose a reason for hiding this comment

Naios commented Jun 19, 2017 •

edited

Naios Jun 19, 2017 •

edited

Naios commented Jun 19, 2017 •

edited

Naios commented Jun 19, 2017 •

edited

Naios commented Jun 20, 2017 •

edited

Naios commented Jun 25, 2017 •

edited