Use const instead of constexpr in Ellipsoid constructor

[adlarkin]

Signed-off-by: Ashton Larkin [email protected]

🦟 Bug fix

Summary

While working on gazebosim/sdformat#821, I had the following build error:

In file included from /usr/include/ignition/math6/ignition/math/Ellipsoid.hh:132:0,
                 from /sdf_to_usd/src/sdformat/include/sdf/Ellipsoid.hh:20,
                 from /sdf_to_usd/src/sdformat/src/Ellipsoid_TEST.cc:19:
/usr/include/ignition/math6/ignition/math/detail/Ellipsoid.hh: In instantiation of 'T ignition::math::v6::Ellipsoid<Precision>::Volume() const [with Precision = double]':
/sdf_to_usd/src/sdformat/src/Ellipsoid_TEST.cc:27:3:   required from here
/usr/include/ignition/math6/ignition/math/detail/Ellipsoid.hh:99:43: error: '(1.2566370614359172e+1 / 3.0e+0)' is not a constant expression
   constexpr T kFourThirdsPi = 4. * IGN_PI / 3.;

I believe the issue is with IGN_PI. It is defined as a floating point value, but the c++ standard says the following (source):

a variable is usable in constant expressions at a point P if the variable is: 
- a constexpr variable, or
- it is a constant-initialized variable
    - of reference type or
    - of const-qualified integral or enumeration type

So, since IGN_PI is not a const-qualified integral/enumeration type, I believe this is causing an error in the sdformat ellipsoid unit test (I'm not sure why this hasn't been an issue until now, though).

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #366 (c4a050f) into ign-math6 (fa67ea8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           ign-math6     #366   +/-   ##
==========================================
  Coverage      99.65%   99.65%           
==========================================
  Files             67       67           
  Lines           6380     6380           
==========================================
  Hits            6358     6358           
  Misses            22       22           

Impacted Files	Coverage Δ
include/ignition/math/detail/Ellipsoid.hh	100.00% <100.00%> (ø)

---

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 fa67ea8...c4a050f. Read the comment docs.

[azeey]

I think it is possible to use IGN_PI in a const expression because it's a macro, not a variable. I've tested it on godbolt and here's an answer that elaborates how the "const-qualified integral or enumeration type` statement is to be interpreted: https://stackoverflow.com/questions/58514970/constexpr-int-and-constexpr-double-in-c

I suspect there might be something else wrong causing the error. Does the error happen in CI in sdformat?

[adlarkin]

I suspect there might be something else wrong causing the error. Does the error happen in CI in sdformat?

For the sdformat PR I was testing that had this error, CI currently isn't working because the PR adds a new dependency and needs to be updated to use the ign-cmake code (gazebosim/sdformat#736). So perhaps it's something in that PR that's causing the issue, but I can't say for sure (the PR is large and I am just now starting to review it). Is there a reason why constexpr would be preferred over const in this case?

[azeey]

The generated code looks exactly the same, so I don't think one would be preferred over the other, but the fact that it's causing an error somewhere else makes me suspect there's something else going on. This code has been released and CI has been running the Ellipsoid_TEST in sdformat for a while without any issues, so I'd be curious to know what change caused the error.

[adlarkin]

but the fact that it's causing an error somewhere else makes me suspect there's something else going on. This code has been released and CI has been running the Ellipsoid_TEST in sdformat for a while without any issues, so I'd be curious to know what change caused the error.

I agree. What's weird too is that the PR I was testing does not touch the ellipsoid code or test at all. The ultimate goal is to break up gazebosim/sdformat#736 into several smaller PRs (like what's being done with gazebosim/sdformat#817), so once that happens, maybe we can catch anything strange that's going on in these new sdformat additions.

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1