ros / urdf

Repository for URDF parsing code
63 stars 41 forks source link

Switch to using generate_export_headers instead of hand-coded visibility_control.hpp #11

Closed clalancette closed 1 year ago

clalancette commented 6 years ago

The CMake-3 way to do export symbols (attribute('default') on Linux, declspec(dllexport) on Windows) is to use a function called generate_export_headers (documentation). The basic pattern would be to put something like this in CMakeLists.txt:

include(GenerateExportHeader)

generate_export_header(${PROJECT_NAME})

which would generate a file called urdf_export.h. In theory, cmake generates the correct export symbols for the platform. From there, urdf_export.h needs to be included in each file that needs the export declarations, and the symbols to be exported need to be marked with URDF_EXPORT (this may already be done).

wjwwood commented 6 years ago

One thing I'll say about this approach is that relying on generated code to make pretty much all of your headers compilable is annoying in editors that try to do code completion, since you often are working from a source copy and don't have the generated code on your path. On the other hand, if you write this file yourself or generate it once and commit it, then you don't have this problem. My experience has been that these files basically don't change (or if they do they don't need to do so) build to build.

Also, you now need cmake for generating code that is required for your package to compile. This happens in other cases too, but if avoidable is nice to do so because it makes your source stand on it's own better, and for instance if someone wants to rewrite this package to use bazel or pure make they can do that more easily.

Neither of the above are reasons to not use generate_export_headers by themselves, but I think they're worth noting.

ijnek commented 2 years ago

Just wondering, what's the currently recommended method? I still see visibility_control.hpp being used in a lot of projects still.

clalancette commented 2 years ago

Just wondering, what's the currently recommended method? I still see visibility_control.hpp being used in a lot of projects still.

Despite the fact that I originally opened this issue, I think the hand-coded visibility_control.h file is recommended (and preferable). We only use this generate_export_headers in a couple of projects.

ijnek commented 2 years ago

Got it, thanks @clalancette I'll follow your recommendation in another project. Perhaps this issue should be closed then?

clalancette commented 1 year ago

Got it, thanks @clalancette I'll follow your recommendation in another project. Perhaps this issue should be closed then?

Yeah, I agree. I'm going to close this out.