Refactor the C++ version of unique_id so it can be included in more than one cpp file

[corot]

Just put "inline" on each function should do the trick, or (more tedious) separate the implementation in a cpp. Otherwise, it's a pain to use is in multiple source files.

Or am I missing the right use of it? In that case, please feel free to close this, and... ideally give me a solution 🙏

[jack-oquin]

Thanks for the report.

This looks like a bug, and none of my test cases catch it. I'll try to construct a test for it, then fix it when I can demonstrate the error.

Your "inline" suggestion makes sense to me. None of those functions seem large enough to be worth creating a separate .cpp file and a shared library to contain it.

[jack-oquin]

Reproducing this bug is easy: adding another .cpp file to the test_unique_id executable with an include of unique_id.h produces the expected linker failure.

[corot]

Well, I tried the inline approach and I got some difficulties for compiling (I don't remember what errors exactly, but was related to the underlying boost implementation), so I separated implementation and headers:
https://github.com/corot/world_canvas_libs/blob/master/world_canvas_client_cpp/include/world_canvas_client_cpp/unique_id.hpp

[jack-oquin]

Yes. I think you're right. The problem is that impl/unique_id.h instantiates several boost function and data objects, which really requires a separate compilation unit, like the solution you came up with. That approach does separate the interface from the implementation more cleanly.

That will work for Indigo. But, I am reluctant to release an ABI-breaking change like that to Hydro at this point.

I suppose we could consider releasing it to Hydro anyway, if necessary. Since no one had previously reported this bug, they must have only included unique_id.h in a single C++ compilation, so maybe they will not notice the binary interface change. But, it might break build compatibility for packages that need to add or modify a target_link_libraries() statement.

[jack-oquin]

According the ROS wiki page "used by" links for Hydro and Indigo, the only indexed packages using this one are ROCON components and also the geodesy package. Most of those references are in Python, not C++. So, we can probably release this fix to both distributions, if desired.

[jack-oquin]

I finally got back to working on this problem.

Declaring everything static solves the problem for my tests. I also made the API functions inline as you suggested.

Please run your test with this fix and let me know if it solves the original problem. If so, I will release it to both Hydro and Indigo.

[jack-oquin]

@corot: does the latest master using static inline fix your problem?

[jack-oquin]

Even without further test verification, I am releasing this to Indigo and Jade.

Do you need a Hydro release, too?

[jack-oquin]

Fixed for Indigo and Jade in release 1.0.5.

[corot]

Hi @jack-oquin, sorry for the delay. Works fine now, so I removed the wrapper. Thanks!

[jack-oquin]

Glad it fixes your problem. I released it to Indigo and Jade, but not Hydro.

Do you need a Hydro release for this?

[corot]

We use indigo, so no problem for us. I think you don't need to bother to release on hydro

[jack-oquin]

Sounds good. Since Hydro is near end-of-life I see no need to release it there.