Fix std::function disambiguation in Windows #27

esteve · 2015-04-29T22:59:52Z

Workaround for http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2132 in Windows

…ons of create_service. Workaround for http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2132 in Windows

dirk-thomas · 2015-04-29T23:19:41Z

rclcpp/include/rclcpp/node.hpp

+    FunctorT,
+    2,
+    typename rclcpp::service::Service<ServiceT>::SharedPtr
+    >::type


The indentation looks difficult to "parse". Would one of the following be clearer?

typename function_arity < FunctorT, 2, typename rclcpp::service::Service<ServiceT>::SharedPtr >::type

or

template<typename ServiceT, typename FunctorT> typename function_arity< FunctorT, 2, typename rclcpp::service::Service<ServiceT>::SharedPtr>::type

I ended reformatting this code with the second style.

wjwwood · 2015-04-29T23:24:38Z

Can you explain this a little bit? I'm confused as to why the return type of create_service had to be rolled into this. I can see why the additional FunctorT template variable is needed, but it isn't clear to me how the return type is involved.

If we want to merge this I think we should take some time to make it cleaner, focusing if possible on the signature of the method, which is what the users will be exposed to. Maybe we could do a dispatch function which takes either callback and then calls an implementation function which has all of this template magic added. That way the user facing API isn't as busy looking. @esteve do you think it is possible to hide some of this in the implementation?

esteve · 2015-04-29T23:39:00Z

@wjwwood this basically checks that the callback passed as the functor argument either has two or three arguments (without and with the request header). I tried getting this working with std::enable_if and std::is_convertible, but unfortunately VS still thinks that the two callback types are interchangeable. My guess is that it's related to "Q. When will you implement Expression SFINAE?" in http://blogs.msdn.com/b/vcblog/archive/2015/04/29/c-11-14-17-features-in-vs-2015-rc.aspx

The return type is needed in the function_arity template because it's used in std::enable_if so that the templated function can return an appropriate return type.

I agree with making this more readable and expressive, the generated Doxygen docs will look horrible as it is now. I think we could change this a little bit so that there's only one create_service method, that gets passed a std::function with variadic arguments and check the number of arguments and their type inside the create_service method.

…n the arity of the callback function

esteve · 2015-04-30T16:12:17Z

I refactored the code a little bit and now create_service is not as ugly as it was, it dispatches internally to an appropriate version of create_service_internal depending on the number of arguments of the callback passed to it. We could check the type of the arguments of the callback, but maybe this is enough.

esteve · 2015-04-30T19:22:07Z

I ended up adding support for checking the types of the arguments of the callback passed as well, this should make create_service more robust.

esteve · 2015-04-30T19:26:37Z

@dirk-thomas @tfoote @wjwwood when you have a moment, please have a look at the latest changes. The Windows slave builds fine now: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_windows/212/

Thanks.

dirk-thomas · 2015-04-30T20:34:05Z

rclcpp/include/rclcpp/node.hpp

-
-  /* Create and return a Service. */
-  template<typename ServiceT>
+  template<typename ServiceT, typename F>


Can we rename F to something "longer" like CallbackT?

I agree that a more verbose name would be good.

dirk-thomas · 2015-04-30T20:36:41Z

Looks much cleaner to me: +1

…e a request header

esteve · 2015-04-30T21:17:09Z

I had to remove the check for the third argument for callbacks that take a request header because it failed with simpler callbacks. Apparently checking for arity is not enough and std::enable_if evaluates every clause.

esteve · 2015-04-30T21:17:43Z

This is the code I had to disable:

02a41cd

wjwwood · 2015-04-30T22:07:30Z

+1 The API looks much cleaner now.

Fix std::function disambiguation in Windows

tfoote · 2015-04-30T22:15:55Z

lgtm

dirk-thomas · 2015-04-30T22:16:21Z

+1

Please squash before merging.

* ros2GH-61 Read topic directly from message when playing and allow to play multiple topics * ros2GH-61 Add test for SqliteStorage and update old ones * ros2GH-62 Extend function to poll for any number of specified topics * ros2GH-62 Allow subscription to several topics * ros2GH-61 Obtain the topic name directly from the database - Uses a JOIN instead of mapping the topic_id to the name in code * ros2GH-61 Cache read row in result iterator This allows repeated dereferencing on same row without quering the database again. * ros2GH-62 Change demo-record to allow specifying multiple topics * ros2GH-62 Add test to write non-string topic + refactoring * ros2GH-62 Add test for subscription to multiple topics * ros2GH-62 Cleanup * ros2GH-62 Simplify test setup * ros2GH-61 Cleanup * ros2GH-61 consolidate storage integration test * ros2GH-62 Consolidate write integration tests * ros2GH-61 enhance read integration test to check multiple topics * ros2GH-62 Improve rosbag integration test * ros2GH-62: Polish rosbag2_rosbag_node_test * ros2GH-62 Fix cpplint * ros2GH-62 Fix memory leak in rosbag helper * ros2GH-62 Cleanup of subscriptions * ros2GH-62 do not use flaky timers in rosbag2_write_integration_test * ros2GH-62 Use rmw_serialize_message_t consistently in test helper classes * ros2GH-73 Use test_msgs in read_integration_test * ros2GH-26 Cleanup: fix alphabetic orderung

* ros2GH-27 Implement discovery for new topics after startup - this allows new topics to be discovered also after startup. - as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup - each time the recorder subscribes to a new topic, this will be logged to console * ros2GH-27 Provide polling frequency as option - make option available in RecordOptions - set to 100ms for now * ros2GH-27 Stop topic polling if subscription setup is complete * ros2GH-27 Minor refactoring for readability * ros2GH-27 Fix build after rebase * ros2GH-145 add no-discovery option to ros2 bag record * ros2GH-27 Refactor subscribing mechanism - use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed * ros2GH-27 Expose polling-interval as cli option * ros2GH-27 Minor refactoring * ros2GH-27 Refactor recorder - naming of methods - emphasize similarity between first subscription and discovery loop * small touch ups * remove launch from function name

Check callback arity and enable/disable compatible/incompatible versi…

2367549

…ons of create_service. Workaround for http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2132 in Windows

esteve added the in progress Actively being worked on (Kanban column) label Apr 29, 2015

dirk-thomas mentioned this pull request Apr 29, 2015

Remove static_cast ros2/examples#21

Merged

dirk-thomas reviewed Apr 29, 2015
View reviewed changes

Esteve Fernandez added 3 commits April 29, 2015 16:40

Formatting

db0238c

Let the compiler deduce the appropriate Service constructor to use

ad3f5da

Delegate service creation to SFINAE'd create_service_internal based o…

1181daf

…n the arity of the callback function

Check callback argument types

814f410

Moved std::enable_if to template parameters

faba591

esteve force-pushed the callable-compatible branch from 639865d to faba591 Compare April 30, 2015 20:22

dirk-thomas reviewed Apr 30, 2015
View reviewed changes

Esteve Fernandez added 3 commits April 30, 2015 14:11

Use longer name

bbd7e39

Fix code style

805d5cb

Removed check for third argument, fails with callbacks that don't tak…

02a41cd

…e a request header

esteve added a commit that referenced this pull request Apr 30, 2015

Merge pull request #27 from ros2/callable-compatible

a5138a2

Fix std::function disambiguation in Windows

esteve merged commit a5138a2 into master Apr 30, 2015

esteve removed the in progress Actively being worked on (Kanban column) label Apr 30, 2015

esteve deleted the callable-compatible branch April 30, 2015 22:15

hidmic mentioned this pull request Jun 26, 2020

Fix pub/sub count API tests. #1203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix std::function disambiguation in Windows #27

Fix std::function disambiguation in Windows #27

esteve commented Apr 29, 2015

dirk-thomas Apr 29, 2015

esteve Apr 29, 2015

wjwwood commented Apr 29, 2015

esteve commented Apr 29, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

dirk-thomas Apr 30, 2015

wjwwood Apr 30, 2015

esteve Apr 30, 2015

dirk-thomas commented Apr 30, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

wjwwood commented Apr 30, 2015

tfoote commented Apr 30, 2015

dirk-thomas commented Apr 30, 2015

Fix std::function disambiguation in Windows #27

Fix std::function disambiguation in Windows #27

Conversation

esteve commented Apr 29, 2015

dirk-thomas Apr 29, 2015

Choose a reason for hiding this comment

esteve Apr 29, 2015

Choose a reason for hiding this comment

wjwwood commented Apr 29, 2015

esteve commented Apr 29, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

dirk-thomas Apr 30, 2015

Choose a reason for hiding this comment

wjwwood Apr 30, 2015

Choose a reason for hiding this comment

esteve Apr 30, 2015

Choose a reason for hiding this comment

dirk-thomas commented Apr 30, 2015

esteve commented Apr 30, 2015

esteve commented Apr 30, 2015

wjwwood commented Apr 30, 2015

tfoote commented Apr 30, 2015

dirk-thomas commented Apr 30, 2015