search

ros-geographic-info / unique_identifier

ROS support for Universally Unique Identifiers
http://ros.org/wiki/unique_identifier
16 stars 19 forks source link

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

Closed corot closed 9 years ago

corot commented 10 years ago

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 :pray:

jack-oquin commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 9 years ago

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 commented 9 years ago

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

jack-oquin commented 9 years ago

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

Do you need a Hydro release, too?

jack-oquin commented 9 years ago

Fixed for Indigo and Jade in release 1.0.5.

corot commented 9 years ago

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

jack-oquin commented 9 years ago

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

Do you need a Hydro release for this?

corot commented 9 years ago

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

jack-oquin commented 9 years ago

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