jwnimmer-tri
commented
3 years ago The Vector classes might be affected by this too.
Yes, I believe that Vector3<T>::UnitX etc. could all affected, at least in theory. (Some compilers will optimize away the problematic portions, in practice.)
The full list of static const globals on the default branch seems to be: Angle, Color, Material, Matrix3, Matrix4, Pose3, Quaternion, Vector2, Vector3, Vector4.
Note that Material::Predefined() is particularly problematic, because it returns to the user a reference to std::map, which always implies a heap allocation and thus will always be leaked anytime the library is loaded and then unloaded. Similarly for kMaterialData in src/MaterialType.hh. Even if those collection types are fixed to be heapless, the class Material has a name which is required to always be on the heap.
Also note that both construction and destruction order are in play here. As library code, I'd argue that ign-math should have no global constructors nor global destructors registered at all.
For cross-reference, see also https://github.com/ignitionrobotics/sdformat/issues/552.
A simple fix is ...
A third option (similar to the first option in the OP) would be use function-local static std::aligned_storage, which still keeps the data in static global storage, instead of on the heap. Once example is Drake's never_destroyed implementation. This can be useful if the constructor is complicated enough that it cannot be made constexpr. For this option as well as the first one, the function-local static requires the compiler to emit a lock guard instruction and storage for the boolean lock, so that the constant will be latch-initialized upon first use. This is useful for complicated globals, but is overkill for the simple structs such as vector, color, quaternion, etc.
A fourth option (similar to the second option in the OP) is to keep the member fields static, but instead mark them constexpr at the definition. See PR #283 for one example. This is ABI-breaking but not API-breaking. Edit: This does not work on Windows vs dllexport. Therefore, I've tried option (4b) now -- use const references + constexpr storage.
Therefore, my suggestion is to use:
- option (4) for the simple numeric constants (angle, vector3, vector4, pose, quaternion, color) as in PR #283;
- either option (1) or option (3) for any global data that is const but cannot be constexpr (material);
Hopefully, there are no cases of mutable global data.
azeey
richmattes
mjcarroll
scpeters
In
ignition::math::Colorthe color constants are defined likepublic: static const Color Cyan;. While the resulting interface is nice to use, it could cause static initialization order fiasco as C++ makes no guarantee that global variables in this form is going to be initialized in usage order. In other words, another global variable could useColor::Cyanand be initialized first.A simple fix is to convert to a function form like the following:
public: const Color& Cyan() { static auto *c = new Color(0, 1, 1, 1); return *c; }Another solution is more involved but it could retain the ability to refer to constants without parentheses.
public: static constexpr Color Cyan = {0, 1, 1, 1};Both solutions represent a API/ABI change, so I would like to hear some feedback first before sending out PRs.
The Vector classes might be affected by this too.