Integration of C/C++ rosidl_typesupport

[kunaltyagi]

https://github.com/ros2/ros2/issues/424#issuecomment-348835770

[dirk-thomas]

I don't understand why several of the files are being moved from the c/cpp package into the common package without being changed. Why should they not stay in the c/cpp package if they contain only code which is specific to the language?

[kunaltyagi]

The code is so similar that using macro, I can modify it for C/C++. For example:

• using namespace__var_name in C and namespace::var_name in C++
• using if __cplusplus extern "C" #endif in C and namespace some_namespace in C++

So, I simply loop over all the files, changing only one macro

The package is neither dependent on the c/cpp package nor they depend on it, instead it replaces both. The code in both packages is so small and common (yet different) that any package for both of them to depend upon would be nearly empty

[dirk-thomas]

While some parts of the patch makes sense to deduplicate code I think you are trying to apply the pattern for all cases. Imo e.g. the "generalization" of the following files makes them much harder to read than the language specific versions:

• src/message|service_type_support_dispatch.cpp

I also don't see why any of the following files have been moved. Imo they should all stay in the c|cpp packages since they contain language specific code and are not "common" at all:

• bin/rosidl_typesupport_c|cpp
• cmake/rosidl_typesupport_c|cpp_generate_interfaces.cmake
• rosidl_typesupport_common_c|cpp-extras.cmake.in

[kunaltyagi]

• bin/rosidl_typesupport_c|cpp

The code is same. I didn't know if the files have to be kept duplicated in each project or not. I'll remove the other files.

I think you are trying to apply the pattern for all cases

Yeah. The files were quite similar.... so I went ahead. I'll undo those changes.

Should I also remove the CMakeLists.txt from this folder?

[dirk-thomas]

Should I also remove the CMakeLists.txt from this folder?

No, the common package will need one in order to be build, install its files and be findable by the c|cpp packages.

[kunaltyagi]

Does this direction make more sense? Also, how should the CMakeLists.txt and package.xml look like for the common folder? Any similar packages out there which can help?

Is there a tutorial for hand-crafting package.xml since auto-creation is yet to be added (as per the webpage)?

[dirk-thomas]

The current diff is still almost impossible to read. Please try to describe the proposed changes in a comment with bullets like:

• create a new common package
• since the content of A and B is almost the same move it to C in the new package
• then do this
• ...

Please try to be as detailed as possible and make each bullet as simple as possible.

Currently several changes are unclear / wrong in the patch. E.g. you can't just remove the package specific visibility macros. The remaining code is still using them, e.g. ROSIDL_TYPESUPPORT_CPP_PUBLIC. Before spending more time on changing the patch I think we should find a consensus on the proposed changes as described above.

[dirk-thomas]

@kunaltyagi What is the status of this? Are you planning on working on this in the future?

[dirk-thomas]

I am going to close this due to no response. Please feel free to comment and the ticket can be reopened.

[kunaltyagi]

So, I got back to this after a long time.

From my perspective, a lot of code is shared between the two packages, and this might be because the languages are related. But that's kinda the point. Remove the code that's repeated or and replace it with dependency on a common package.

Unless I'm mistaken and the point is to make a common package so that rosidl_typesupport_rust or something else might benefit too. In that case, this is much more complicated than I think this is.

I've force pushed to clean the history. There are just 2 commits which I believe show the starting point.

1. Creation of a new package, rosidl_typesupport_common with an incomplete CMakeLists.txt (which will get updated later as the correct stuff gets added to this package). The common dependencies have been copied to package.xml
2. Added the files with a lot of common content depending only on the package name. Since the content is smaller than last time, it should be easy to point out where this approach is wrong.

Future:

• Make this dependency of c/cpp package
• Remove the common files and use the function/macro from the common package instead

[dirk-thomas]

Please make sure to maintain the history of moved code. You can do that by using separate commits for:

• copying (but not modifying) a file
• making change to the copied file
• update/remove previously existing code to use the modified file

Also files like the visibility macro can not be shared since each package / library needs its own macros in order to toggle them independently.

[kunaltyagi]

Got it. I'll modify the history to fit this process.

So the only header file that can be shared is the type_support_map?

EDIT: I did a pretty big shakeup of the history. Only retained the bin directory and modified it to add an argument for the package whose generator will be used. I think this keeps the generation process independent of the language (not just c/cpp) despite the fact that generate_* function were pretty much the same