Deduce MessageT from the first argument of the callback

[esteve]

This PR removes the need to specify twice the type of a created subscription by deducing the message type from the callback.

This is a quick hack that came up from the discussion in https://discourse.ros.org/t/rfc-using-c-14/921/10

[esteve]

Awww, this fails to compile with allocator_example.cpp, moved back to "in progress"

[esteve]

D'oh! It was as easy as fixing the order in which the template arguments are passed to Node::create_subscription 444c8e0, I feel so dumb 😝

[dirk-thomas]

I would propose to first update to C++14 (as being discussed on discourse), then remove as many custom function traits as possible, and then add additional ones like this if necessary.

[wjwwood]

Does C++14 affect the template evaluation behavior? I don't see why that would block this. I could understand testing if we need func traits with vs2015 update 3 before merging this, but even then, it seems like a weak argument to hold off on this since it would be an incremental improvement.

It might conflict with changes in my pr also. #277

[dirk-thomas]

From my experience it is always easier to first clean up / remove unnecessary code and then add new features (rather then the other way around). That being that there is no reason it couldn't be approached the other way around.

I would leave the decision about the order to the person who will do the switch to C++14 and the cleaning up of the function traits since it affect their effort / complexity.

[wjwwood]

Sorry, I still don't understand what link is between function traits and C++14?

[dirk-thomas]

With the availability of C++14 the idea is that some of the traits will be obsolete. Some of them have only been introduced to work around compiler limitation on Windows in the first place.

[wjwwood]

But the move to C++14 doesn't involve changing the Windows compiler, AFAIU. I don't think C++14 introduces any feature that replaces the need for function traits. As I understood it, they aren't even necessary for the most part with GCC and Clang.

[dirk-thomas]

That is correct. The thing that has changed is not the usage of C++14 but the update to Visual Studio 2015 Update 3. With the updated compiler a few of the traits should be obsolete.

[wjwwood]

Since we've not started that work yet (testing the continued need for function traits now that we have VS2015 update3) and this is ready to go now, I'd vote to merge it.

[codebot]

I vote for merge as well; this is a very nice improvement to user-facing syntax.

[wjwwood]

@codebot let me see about the conflicts between this and my pr #277 before merging this. I'll fix this one up if my pr being merged breaks it.

[esteve]

@wjwwood I've squashed the commits in this branch and in ros2/examples#146, let me know if you want me to rebase this on top of #277. Additionally, any code that calls Node::create_subscription needs to be updated (e.g. demos, system_tests, etc.), so this PR can't be merged right now until the other repositories are updated as well. I'll probably have time to create branches for the other repositories in the next couple of days, unless you do it first :-)

@dirk-thomas I'm sorry, but I fail to see what's the relationship between switching to C++14 and this PR, I don't see how merging this precludes migrating to C++14. Especially because the language changes in C++14 https://en.wikipedia.org/wiki/C%2B%2B14#New_language_features don't have any effect on this code or the existing code in function_traits.hpp (both use features from the language, not the standard library). In any case, it'd be great to have function_traits.hpp removed (or greatly reduced) if switching to VS2015 Update 3 fixes the ambiguity issues with std::function

@codebot 🍔 🌭 🐔

[esteve]

BTW, I think this same pattern could probably be reused with services to remove the need to explicitly pass the service class to Node::create_service as a template argument

[dirk-thomas]

Since the template arguments seems to not be optional (otherwise existing code wouldn't need to be updated) how will this work in the future when the callback signature might use a different type (aka type masquerading)?

[wjwwood]

That's actually a good point, that the swapped order of the template arguments will break anyone who wants to be explicit about the type, i.e. anyone who still wants to do auto sub = n->create_subscription<std_msgs::msg::String>(...). @esteve can you shed some light on why the CallbackT must come before the MessageT for this to work?

[esteve]

@wjwwood sure, it's because MessageT is deduced from the CallbackT template which needs to be available before MessageT is evaluated, but also because template arguments with default values should appear after those that don't have one, unless you want both to have the same default value (http://en.cppreference.com/w/cpp/language/template_parameters#Default_template_arguments)

I've rebased this branch and also added support for explicitly passing MessageT to Node::create_subscription in (9d67d5e), to maintain compatibility with the current API and also for supporting type masquerading in the future. I've built the rclcpp examples both with and without explicit MessageT successfully on Linux.

PS: I claim no responsibility for @dirk-thomas 's blood pressure when he sees the changes 😜

[codebot]

This is super awesome! 🎉 Thanks for all the work @esteve 🍔 However with the other branches we have in flight at the moment and attempting to code-freeze a few days ago, let's aim to get this in Beta 2.

[esteve]

@codebot thanks! No problem, I just submitted this as a quick experiment to see if it was possible. Best of luck with the beta, go ROS! :-)

[esteve]

Is this ready for merging? I addressed the issue with type masquerading and keeping compatibility with the current API. Also, because create_subscription can take both std::unique_ptr and std::shared_ptr, removing function_traits seems to be rather complex (see #278 (comment)) and updating to the latest version of Visual Studio wouldn't solve anything, because Clang and GCC are affected as well.

[mikaelarguedas]

@ros2/team re-review this please

[esteve]

I've rebased the branch to fix the conflicts, @wjwwood have you had time to have a look at the PR since you self-requested the review?

BTW, if there's no consensus in @ros2/team as to whether the changes should go in or not, I'm totally fine with closing this PR, this was just a quick hack to see if it was possible to omit the template arguments of create_subscription()

[wjwwood]

@esteve no, this is entirely on my plate. I just need to take an hour and review it/test it. It's comes up at most of our meetings :/. I'll get to it eventually.

[esteve]

@wjwwood no worries, I just wanted to say that I'm ok with closing this PR if the changes are out of scope, no hard feelings :-) Let me know if I can help with anything.

[Karsten1987]

looking at this, it is related to the changes in #388. I reference this as a reminder that this PR here can be incorporated in 388.

[wjwwood]

I finally had took some time to review this and the code changes make sense to me!

I did spend a few moments trying to make the changes in ros2/examples#146 work without something like these enable if template arguments (which in turn use the extra function traits), but I was unable to do that using only existing template tools provided by C++14 (nor did I see anything in C++17 that would help specifically).

This would need to be rebased, and we would want to make use of it everywhere, not just in our examples (except where we want to be explicit about which type we're using in the subscription creation).

So tentative code review lgtm, but there is still work to be done (probably by some of us on the ROS team if not myself) before it can be merged. So I'll also move it back into in progress.

[esteve]

@wjwwood I just rebased this branch. At some point I must have removed the branch in ros2/examples#146 Anyway, I've pushed https://github.com/esteve/examples/tree/simplified-templates which removes the type argument of Node::create_subscription in the examples.

[Karsten1987]

Is there a way to explicitly specify the MessageT template? As mentioned, I've done similar things in order to distinguish between a regular callback with MessageT and a raw_message_t.
https://github.com/ros2/rclcpp/pull/388/files#diff-d750d60a7fa9b014bac5623ebeefa4f8

For this to work, I need a MessageT template argument to be specified on the call to Node::create_subscription, because the raw_message_t type does not contain any valuable information (type_support etc) based on the callback signature.

[esteve]

@Karsten1987 you can still pass a MessageT explicitly, see:

https://github.com/esteve/examples/blob/simplified-templates/rclcpp/minimal_subscriber/lambda_explicit_subscription.cpp

however this won't allow you to partially specialize on the MessageT, since create_subscription is a member function of Node. But I see two ways to do it:

• add another std::enable_if in create_subscription that uses std::is_same to check if MessageT is a raw_message_t and act accordingly.

• turn SubscriptionFactory into a templated class and move create_subscription_factory into it as a static method so that you could specialize SubscriptionFactory for MessageT = raw_message_t, so something like this:

template<typename MessageT, typename CallbackT, typename Alloc, typename SubscriptionT>
struct SubscriptionFactory
{
  static
  rclcpp::SubscriptionBase::SharedPtr
  create_typed_subscription(...) {}
};

template<typename CallbackT, typename Alloc, typename SubscriptionT>
struct SubscriptionFactory<raw_message_t, CallbackT, Alloc, SubscriptionT>
{
  static
  rclcpp::SubscriptionBase::SharedPtr
  create_typed_subscription(...) {}
};

(note, I haven't compiled this, but I think it should work).

The first approach is quicker to implement, but I think harder to maintain in the long term. The second one has the advantage of allowing users specialize SubscriptionFactory for their own cases, so they can customize how subscriptions are created for a concrete type.

[cottsay]

@esteve, this change hasn't seen any activity for some time. How would you describe the current status?

[esteve]

@cottsay thanks for the reminder. There's a bunch a of conflicts and I don't feel it's worth resolving then, it'd just be better to write this from scratch. Unfortunately I won't have time to work on it in the foreseeable future, so I'm closing it.