Use constexpr for simple static constants#283
Conversation
I believe we have a we probably have too many other branches so it doesn't show up in the list unless you start typing its name. Yes, if this breaks ABI, then it should target |
|
there seems to be an issue with our windows CI as well |
85edfaf to
aea9284
Compare
Oops! You're right, of course. I've re-targeted the pull request now.
Yup, I'll work on that error using godbolt. (I don't remember the dllimport/dllexport rules for constexpr off the top of my head.) MSVC support is definitely the biggest risk with this approach. I'll see if I can get it working. |
aea9284 to
f303991
Compare
I spent a few hours and couldn't figure out how to get MSVC to obey Instead, I switched this implementation to use const references for the globals, but initialize them using references to constexpr storage. I believe this will be fiasco-safe. I'm seeking input from project maintainers on whether this (#283) or else #286 is your preferred tactic to resolve the fiasco problem. Once I have that, I'll close the dispreferred PR, and update the preferred PR to cover all of the relevant types (Vector2, Vector3, etc.) that use the same pattern. |
f303991 to
3d04dd8
Compare
|
Okay, I've started by rebasing this onto the latest I'll work on porting more classes to this pattern in a future push. |
3d04dd8 to
d76381b
Compare
d76381b to
6a32e30
Compare
|
This should be ready for feedback / review now. |
|
Hi, thanks for working on this. I wonder would the newly added references suffer from initialization order fiasco as well. |
No, I don't think that reference initialization generates any global constructors or destructors. |
scpeters
left a comment
There was a problem hiding this comment.
this LGTM, thanks for your excellent work here!
my one concern is that the ign-math6 branch is very out of date and some new static NaN constants were added to the Vector classes in #222 (released in 6.9.0). I've started merging forward incrementally, starting from the 6.8.0 tag in #305.
Do you want to wait until after we have fully merged forward from ign-math6 and resolve conflicts in this branch or merge this now and fix the NaN implementation when merging forward?
|
I'm content to do it either way, whatever is easiest overall. My goal is to get this fix into Drake's build eventually, but that means a numbered release tag here and porting libsdformat to use it, so nothing here is time-sensitive for me. |
|
According with GDB code is failing here: /// \brief Corrects any nan values
public: inline void Correct()
{
// std::isfinite works with floating point values,
// need to explicit cast to avoid ambiguity in vc++.
if (!std::isfinite(static_cast<double>(this->data[0])))
this->data[0] = 0;
if (!std::isfinite(static_cast<double>(this->data[1])))
this->data[1] = 0;
} |
|
The failing tests are attempting to mutate a reference to One solution to change the test to make a copy like so: nanVec = Ignition::Math::Vector2d.new(Ignition::Math::Vector2d.NaN) |
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
thanks; I've fixed the ruby tests in 279d973 |
Codecov Report
@@ Coverage Diff @@
## main #283 +/- ##
==========================================
- Coverage 99.64% 99.63% -0.01%
==========================================
Files 65 65
Lines 6132 6101 -31
==========================================
- Hits 6110 6079 -31
Misses 22 22
Continue to review full report at Codecov.
|
|
We need a similar change for the failing python tests. Instead of assignment, we need to invoke the copy constructor. e.g. # Instead of
nanVec = Vector2d.NAN
# Use
nanVec = Vector2d(Vector2d.NAN) |
|
I have found a way to make public: %extend {
static Vector2 NaN()
{
return ignition::math::Vector2<T>::NaN;
}
}The generated code allocates a new For pybind11, we can use the .def_readonly_static("NAN", &Class::NaN, py::return_value_policy::copy,
"math::Vector2(NaN, NaN)")If that seems reasonable, I can push my changes to this PR. |
|
I'm surely not the final word here, but for my part I'd say yes -- for languages that don't enforce |
agreed |
This prevents segfaults from happening when users accidentaly try to mutate references to static const members Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
scpeters
left a comment
There was a problem hiding this comment.
looks good to me! I think the complaint about coverage decreasing is a red herring
mjcarroll
left a comment
There was a problem hiding this comment.
Looks good, thanks for iterating @jwnimmer-tri
This fixes the initialization-order-fiasco for static colour constants while preserving API (though breaking ABI) through the following steps: * Make constexpr the OgreColourValue constructor * Instantiate global constexpr variables in a hidden namespace in .cpp file * Change static class constants to references (ABI break)a and assign to global constexpr variables This matches the approach taken with similar static constants in gazebosim/gz-math#283. Reference: - https://abseil.io/tips/140 - https://en.cppreference.com/w/cpp/language/initialization
This fixes the initialization-order-fiasco for static colour constants while preserving API (though breaking ABI) through the following steps: * Make constexpr the OgreColourValue constructor * Instantiate global constexpr variables in a hidden namespace in .cpp file * Change static class constants to references (ABI break) and assign to global constexpr variables This matches the approach taken with similar static constants in gazebosim/gz-math#283. Reference: - https://abseil.io/tips/140 - https://en.cppreference.com/w/cpp/language/initialization
This fixes the initialization-order-fiasco for static colour constants while preserving API (though breaking ABI) through the following steps: * Make constexpr the OgreColourValue constructor * Instantiate global constexpr variables in a hidden namespace in .cpp file * Change static class constants to references (ABI break) and assign to global constexpr variables This matches the approach taken with similar static constants in gazebosim/gz-math#283. Reference: - https://abseil.io/tips/140 - https://en.cppreference.com/w/cpp/language/initialization
🦟 Bug fix
A portion of #269.
Summary
Use constexpr for simple static constants, to avoid the C++ global static construction and destruction order fiascos.
At the moment, only Color has been refactored with this pattern while we discuss the implications.
This is an alternative approach vs #286. In thisversion of the fix, we remain API compatible (but still break ABI). In the #286 version of the fix, we have an API+ABI break (with a deprecation transition), and thus will require users to refactor their code.
This pull request is currently targeted atign-math6, but this is an ABI-breaking change (not API-breaking), so I think this should be targeted tomainorign-math7instead, but those branches do not seem to exist yet.Checklist
Added tests (n/a)Updated documentation (as needed) (n/a)Updated migration guide (as needed) (n/a)codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge