Use constexpr for simple static constants

[jwnimmer-tri]

🦟 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 at ign-math6, but this is an ABI-breaking change (not API-breaking), so I think this should be targeted to main or ign-math7 instead, but those branches do not seem to exist yet.

Checklist

• Signed all commits for DCO
• Added tests (n/a)
• Updated documentation (as needed) (n/a)
• Updated migration guide (as needed) (n/a)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[scpeters]

This pull request is currently targeted at ign-math6, but this is an ABI-breaking change (not API-breaking), so I think this should be targeted to main or ign-math7 instead, but those branches do not seem to exist yet.

I believe we have a main branch:

• https://github.com/ignitionrobotics/ign-math/tree/main

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 main

[scpeters]

there seems to be an issue with our windows CI as well error C2491: 'ignition::math::v6::Color::White': definition of dllimport static data member not allowed

C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(351): error C2491: 'ignition::math::v6::Color::White': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(352): error C2491: 'ignition::math::v6::Color::Black': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(353): error C2491: 'ignition::math::v6::Color::Red': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(354): error C2491: 'ignition::math::v6::Color::Green': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(355): error C2491: 'ignition::math::v6::Color::Blue': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(356): error C2491: 'ignition::math::v6::Color::Yellow': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(357): error C2491: 'ignition::math::v6::Color::Magenta': definition of dllimport static data member not allowed
C:\Jenkins\workspace\ignition_math-ci-pr_any-windows7-amd64\ws\ign-math\include\ignition/math/Color.hh(358): error C2491: 'ignition::math::v6::Color::Cyan': definition of dllimport static data member not allowed

[jwnimmer-tri]

I believe we have a main branch ...

Oops! You're right, of course. I've re-targeted the pull request now.

there seems to be an issue with our windows CI ...

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.

[jwnimmer-tri]

there seems to be an issue with our windows CI ...

MSVC support is definitely the biggest risk with this approach.

I spent a few hours and couldn't figure out how to get MSVC to obey inline (as implied by constexpr) on a storage definition, when mixed with dllexport. I can't tell if its a bug in MSVC or my understanding of how dllexport is supposed to work. In any case, playing compiler games is probably not the path we want to take here, so I abandoned that approach.

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.

[azeey]

Per my comment in the issue, we prefer #283 over #286 as it maintains API stability.

[jwnimmer-tri]

Okay, I've started by rebasing this onto the latest main and finishing up the port for Color.

I'll work on porting more classes to this pattern in a future push.

[jwnimmer-tri]

This should be ready for feedback / review now.

[mahiuchun]

Hi, thanks for working on this. I wonder would the newly added references suffer from initialization order fiasco as well.

[ahcorde]

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;
      }

[scpeters]

@ahcorde I've cherry-picked #280 and #285 to main and resolved conflicts. Can you take a look if there are test failures?

[azeey]

The failing tests are attempting to mutate a reference to Vector*d::NaN. For example, in Vector2_TEST.rb,
https://github.com/ignitionrobotics/ign-math/blob/080eb34dde41243fded76bbdf67377192f6044ef/src/ruby/Vector2_TEST.rb#L262-L268
nanVec is assigned Vector2d.NaN, but this assignment only creates a reference to Vector2d.NaN, so mutating nanVec is equivalent to mutating Vector2d.NaN and causes a segfault because the symbol associated with Vector2d.Nan is in the read only data section of the shared library.

One solution to change the test to make a copy like so:

    nanVec = Ignition::Math::Vector2d.new(Ignition::Math::Vector2d.NaN)

[scpeters]

One solution to change the test to make a copy like so:

    nanVec = Ignition::Math::Vector2d.new(Ignition::Math::Vector2d.NaN)

thanks; I've fixed the ruby tests in 279d973

[codecov]

Codecov Report

Merging #283 (cb523e1) into main (280c00b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
include/ignition/math/Pose3.hh	100.00% <ø> (ø)
src/Angle.cc	89.09% <ø> (-0.57%)	⬇️
src/Color.cc	100.00% <ø> (ø)
include/ignition/math/Angle.hh	100.00% <100.00%> (ø)
include/ignition/math/Color.hh	100.00% <100.00%> (ø)
include/ignition/math/Matrix3.hh	100.00% <100.00%> (ø)
include/ignition/math/Matrix4.hh	100.00% <100.00%> (ø)
include/ignition/math/Quaternion.hh	99.75% <100.00%> (ø)
include/ignition/math/Vector2.hh	100.00% <100.00%> (ø)
include/ignition/math/Vector3.hh	100.00% <100.00%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 280c00b...cb523e1. Read the comment docs.

[azeey]

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)

[azeey]

I have found a way to make Vector*.NAN return a copy instead of a reference such that the tests pass without any modification (after reverting 279d973). For SWIG, we can use the %extend keyword to create a special function for getting the NaN constant:

      public: %extend {
        static Vector2 NaN()
        {
          return ignition::math::Vector2<T>::NaN;
        }
      }

The generated code allocates a new Vector* object equal to Vector*::NaN

For pybind11, we can use the py::return_value_policy::copy as described here

    .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.

[jwnimmer-tri]

I'm surely not the final word here, but for my part I'd say yes -- for languages that don't enforce const, having the global constants avoid mutation accidents by always returning a new copy sounds good to me.

[scpeters]

I'm surely not the final word here, but for my part I'd say yes -- for languages that don't enforce const, having the global constants avoid mutation accidents by always returning a new copy sounds good to me.

agreed

[scpeters]

looks good to me! I think the complaint about coverage decreasing is a red herring

[mjcarroll]

Looks good, thanks for iterating @jwnimmer-tri