C++: types.inl make functions inline

[filippobrizzi]

Changelog

Make functions defined in types.inl inline.

Description

This is needed to avoid linker multiple definition errors.

How to reproduce:

• Create a library with two .cpp files
• One file includes reader.hpp and one writer.hpp and both need to define MCAP_IMPLEMENTATION
• File types.inl is included by both compilation units and when linker creates the libraries, the functions from types.inl are defined twice.

[filippobrizzi]

PTAL @indirectlylit

[jtbandes]

I believe this usage is incorrect — MCAP_IMPLEMENTATION shouldn't be defined in more than one TU. This is documented in the example at https://github.com/foxglove/mcap/tree/main/cpp:

#define MCAP_IMPLEMENTATION  // Define this in exactly one .cpp file

[filippobrizzi]

I believe this usage is incorrect — MCAP_IMPLEMENTATION shouldn't be defined in more than one TU. This is documented in the example at https://github.com/foxglove/mcap/tree/main/cpp:

#define MCAP_IMPLEMENTATION  // Define this in exactly one .cpp file

@jtbandes @jhurliman That is not true. If I define it only in one TU where for example I include writer.hpp, in the other TU where I includereader.hpp it will not include reader.inl and I will have missing symbol.

One solution would be to include both reader.hpp and writer.hpp in one TU to pull the definitions (even if not needed), but that would be a trick to overcome a bad design.

[jtbandes]

Can you explain more about why you think this design is bad?

[filippobrizzi]

Can you explain more about why you think this design is bad?

@jtbandes sorry, you are right, I should have given more context.

• As pointed out above there are issues when you have files included by different top-level ones (see types.hpp that is included by both reader.hpp and writer.hpp). With the current design, there is no alternative to making all functions in types.inl inline.
• With the current design you are trying to get the benefits of a header-only library without paying its compile-time cost.
  • This, however, adds complexity to the user who needs to understand the intricacy of a design choice that is not a common practice (honestly, I can't think of another library that uses the same pattern).

That said, the second point is just my opinion, feel free to ignore it, but this PR is still necessary otherwise I cannot compile my code.

I hope what I wrote is clear, otherwise, I apologize, and please let me know I will be happy to engage more in this conversation.

[indirectlylit]

(just brainstorming here...)

One idea that came up while we discussed #1065 was that the build system story would be simpler if we provided the entire implementation in just two files, e.g. mcap.cpp and mcap.hpp. It sounds like that same strategy might alleviate this issue? (At the cost of not being able to include only reader or writer.)

[indirectlylit]

closing in favor of #1190:

For using MCAP with CMake, the third-party olympus-robotics/mcap_builder repository provides a helpful wrapper.

thanks again @filippobrizzi