math.h and cmath are not completely equivalent, in particular math.h do not properly define all the type-safe variants of the sin, cos function in the std:: namespace.

Even if this features are not used in urdfdom_headers (all the code here uses double variables) I would be a bit afraid to silently change this, that could cause tricky changes in downstream users of urdfdom_headers.

Back in 8fdf48f I added the definition of the _USE_MATH_DEFINES macro, but in retrospective I think is a poor solution, exactly for this kind of problems.

Some alternative solutions could be:

• define and export the _USE_MATH_DEFINES macro at the CMake level,
• avoid to use the M_PI macro at all.

Interesting discussions on this:

• M_PI and other math constants robotology/community#81
• Mathematical (and physical) constants RobotLocomotion/drake#2033

we currently use the version of this PR within our ROS2 base, and it's working fine. Question here is what do you with this PR.

There are several possible options, including this PR itself.

In my opinion the best solution in the short term is to transform the <project>-config.cmake to expose this library as a CMake INTERFACE import library (see Eigen for a good example of this: https://bitbucket.org/eigen/eigen/src/a038e46f2d3e40b6da3af67822d647335ff0fc44/cmake/Eigen3Config.cmake.in?at=default&fileviewer=file-view-default ) and add their (with target_compile_definitions(<target> PUBLIC ...) the _USE_MATH_DEFINES flag. Clearly this solution involves a bit of work both at urdfdom level and in the libraries consuming urdfdom, so it is up to the maintainers decide if it make sense to go for it or not.

In the long term it would make sense to just stop using non-standard definitions (M_PI) in public headers.

M_PI is only used in two places in a single function, so I'd be inclined just to hard-code it there. That risks breaking downstream code that was implicitly including math.h, but that seems easier, since it's just a constant used in one function.

I am fine with explicitly defining M_PI here. What's the recommended procedure to get this PR done?

I think we can revert to using cmath and replace the instances M_PI with a const double

I just reverted my initial commit and defined M_PI as a const double.

Here's an approach I would consider taking:

• scpeters@b0657fc

See #38 for an alternative approach.

I'll close this in favor of #38