alternative way to handle visibility macros

[wjwwood]

@codebot please don't merge this as-is, because it has a header in it that would be placed in rcl instead of this package if others agree this is an ok strategy for simple packages.

I'll paste the docstring from that header here as way of explanation:

/* This header defines a few common visibility macros which change based only
 * on the compiler (MSVC or GCC):
 *
 *   - RCL_EXPORT: declspec statement for dllexport or visibility("default")
 *   - RCL_IMPORT: declspec statement for dllimport or empty
 *   - RCL_LOCAL: empty or visibility("hidden")
 *
 * The way this header is intended to be used is as a simple way to control
 * importing and exporting dll symbols in small, trivial packages.
 * You would first come up with a "public" macro name based on your package
 * name, e.g. if your package was called "my_package" your public macro name
 * might be "MY_PACKAGE_PUBLIC".
 * Then you would place this in each of your header files, after importing but
 * before any code:
 *
 *   #include "rcl/visibility.h"
 *
 *   #ifndef MY_PACKAGE_PUBLIC
 *   #define MY_PACKAGE_PUBLIC RCL_IMPORT
 *   #endif
 *
 * This gives you the public macro, with a default behavior of dll import.
 *
 * You can then use this public macro on all of your functions and class
 * methods like so:
 *
 *   MY_PACKAGE_PUBLIC void my_function();
 *
 * Then you would place this before all includes and code in all your source
 * files (i.e. .c or .cpp files) which include that header:
 *
 *   #include "rcl/visibility_helpers.h"
 *   #define MY_PACKAGE_PUBLIC RCL_EXPORT  // dllexport symbols in header
 *   #include "my_package/my_header.h"
 *   #undef MY_PACKAGE_PUBLIC
 *
 * This would override the default behavior for "MY_PACKAGE_PUBLIC" (which was
 * dll import) to be dll export, which is what you want when compiling the
 * library that implements this header.
 * You can exclude any macro on the definition of your functions and methods:
 *
 *   void my_function() {
 *     // ...
 *   }
 *
 * The RCL_LOCAL definition can be used on functions in your source files to
 * prevent their symbols from showing up in the resulting library, e.g.:
 *
 *   RCL_LOCAL void my_private_function() {
 *     // ...
 *   }
 */

The advantage of this approach is that it avoids having to have a header that is dedicated to visibility in this package. It still results in some boilerplate that is easy to mess up, but the other approach seems equally fragile to me, since they might forget to pass the -D..._BUILDING_DLL=1 to the compiler or something like that. The state of the "public" macro is baked into the code here.

[gavanderhoorn]

This is (completely?) off-topic, I'm not an expert on this and I understand the need, but from reading things like gcc/wiki/Visibility I'm wondering if we're not stepping through some hellish portal to the special ring in ROS2 programming/framework support hell with this.

Are regular users (ie: not contributing users) expected to deal with this as well?

[wjwwood]

@gavanderhoorn it's only important if you want your code to work on Windows. Also, it may be possible to hide everything about a component behind the class loader interface, in which case we may be able to get away with no symbol visibility management at all even on Windows, but that needs some more experimentation to see if that's true. More than likely most people will be able to safely ignore these things, but speaking for myself, I figured the example should be as complete and correct as possible.

[codebot]

Looks awesome 🎉 🌮 but deferred until Beta 2. 😞

[nkoenig]

Need feedback now that the beta release has occurred.

[dirk-thomas]

The approach looks interesting and I would be in favor of putting this header into the new package c_utilities. Based on that it would be interesting to see a set of patches using this new approach across all the ROS 2 packages (especially the typesupport packages).

[Karsten1987]

agreed, c_utilities could be an appropriate place for it.

[wjwwood]

I was originally proposing this for "simple" packages, because while this proposal has a low fixed cost (dependency on package with this general purpose header versus having your own header in your package) it does have a higher variable cost (more boilerplate LOC per file that uses visibility control). So for a package like rclcpp with many headers and C++ files, it would probably be better to just have its own visibility header rather than doing this.

It might be a good solution still for the type support code because that's means fewer files we have to generate per message package.

I'm fine with putting it in the c_utilities package, though I'm a little worried about the length of the new macro names 😄.

I don't have time to do this right now. Maybe we should put this back in ready or close this and open an issue and put it in ready?

[mikaelarguedas]

👍 for closing this and open an issue to add this to c_utilities and start patching packages as a POC