search

ros / roscpp_core

ros distribution sandbox
88 stars 116 forks source link

rostime: removed empty destructor of TimeBase (which makes TimeBase, … #82

Closed cwecht closed 6 years ago

cwecht commented 6 years ago

…Time and WallTime trivially copyable).

This enables the use of these types in contexts where trivial copyability is required, for instance std::atomic or components where memcpy is used.

dirk-thomas commented 6 years ago

Thank you for the patch.

BillWSY commented 5 years ago

Do note this update caused ABI breakage in some systems. We are using some third-party proprietary drivers compiled against the old header, and they stopped to work after release. I was able to hack the binary of the driver to make it work though. There should be a ROS-wide policy about ABI incompatibility.

Also as a side note, since TimeBase is a base class, in many style guide it is supposed to have a virtual destructor or a protected non-virtual destructor. Maybe ideally Time should just be a struct? But I wouldn't recommend making any change to the current interface anymore.

dirk-thomas commented 5 years ago

There should be a ROS-wide policy about ABI incompatibility.

There is no strict policy but we do aim to not break ABI in patch releases. Unfortunately there is currently no automated check in place and it wasn't noticed during the review process that this patch does break ABI.

I am sorry for the inconvenience.