Skip to content
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

Templatize so it works both before and after RMW changes #2

Closed

Conversation

rotu
Copy link

@rotu rotu commented Oct 12, 2019

Note that the extern "C" declarations in this file are quite unnecessary - due to the rmw.h header, the declared interface will have C linkage (though there may be additional overloads that use C++ linkage)

@eboasson
Copy link
Owner

This is awesome! It's a busy day and I haven't had a chance to try it out properly, but let's see if I actually understand what it does ...

If it's the "old" interface, the rmw_subscription_options_t and rmw_publication_options_t don't exist, so there is no reference to functions with the "new" signature, and the templates don't get emitted.

If it is the "new" interface, then defining the old version is an overloaded variant that isn't affected by the extern "C" bit, and so is effectively simply an internal function. But it that case the extern "C" declaration of the new interface forces the expansion of the template.

Or something like that ... I suppose ... I'm not very good with the behaviour of C++ templates ...

Tomorrow I should have a chance to look at it more closely.

@rotu
Copy link
Author

rotu commented Oct 12, 2019

Thanks! I did this because I wanted to learn/practice SFINAE and templates in general.

You’re exactly right. extern “C” specifies which overload gets called when a C program asks for the function by name.

The other gotcha is, even in the template that doesn’t get instantiated, rmw_subscription_t::option doesn’t exist, so you must resolve it via a template type parameter T_rmw_subscription::options instead of the concrete rmw_subscription_t::options

Note that the `extern "C"` declarations in this file are quite unnecessary - due to the rmw.h header, the declared interface will have C linkage (though there may be additional overloads that use C++ linkage)
@rotu
Copy link
Author

rotu commented Oct 13, 2019

Had to change function definition order so the new API functions can call the old API functions. Oops.

@eboasson
Copy link
Owner

@rotu I am very sorry that I have to tell you that on Eloquent with macOS the templating gives problems ... it compiles and links cleanly, but when I try to run something it complains that rmw_create_publisher is not defined in librmw_cyclonedds_cpp.dylib. So I guess that where gcc/ELF/Linux decides to expand the template and provide the definition, clang/Mach-O/macOS decides not to expand it.

Fortunately, there is now this: ros2/rmw#188 (comment). Would you be willing to change rmw_cyclonedds to use that instead?

Then you can get the credit for fixing things, as is only right :)

@rotu
Copy link
Author

rotu commented Oct 17, 2019

I’m on it. I think the weirdness on Mac is that the template definition is in an object file, whereas usually template definitions are in a header file. Oh well. I’ll make the change - it’s a minor modification on my #ifdef based attempt

@rotu rotu closed this Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants