M_PI and other math constants

[pattacini]

Preamble

Presently, we have some mixed configurations in our software to deal with math constants such as M_PI:

1. Some modules define them explicitly via macros, as e.g. #define M_PI 3.1415....

2. Some other modules do more correctly:

   #define _USE_MATH_DEFINES
   #include <cmath>

This way, we might have the following warning, depending on the inclusion order:

'M_PI' : macro redefinition

I'd suggest to always stick to solution 2, which is way more principled.

However, the use of _USE_MATH_DEFINES is tricky, since the developer must pay lots of attention to the order of #include <cmath> macros that may follow throughout different modules. In fact, if the inclusion order is not respected, we might have undefined M_PI in some parts of the code with an huge headache while finding out why (compiler errors do not provide good hints in this respect).

One way to get around this order-related problem is to put in the CMakeLists.txt file the following directive:

add_definitions(-D_USE_MATH_DEFINES)

Problem

With these premises, if I aim to update for example icub-main/ctrlLib to the use of cmath along with _USE_MATH_DEFINES (removing thus the explicit definition of M_PI), I will then easily get the library compiled, but this change will cause very likely errors while compiling code depending on ctrlLib, unless I'll put the same directive add_definitions(-D_USE_MATH_DEFINES) in the corresponding CMakeLists.txt files.

Do we have an automatic way in cmake to export this definition downstream, or is there any other smart solution to this problem?

@drdanz @lornat75 @vtikha @traversaro

[francesco-romano]

This is tricky.
Those constants are not C standard anyway, but are posix extension to C, so in theory even by including math.h you are not guaranteed that those constants are defined.

Regarding the #define _USE_MATH_DEFINES, it is useful for older platforms (see http://stackoverflow.com/questions/1727881/how-to-use-the-pi-constant-in-c).

For example this is the code in my math.h

/******************************************************************************
 *  POSIX/UNIX extensions to the C standard                                   *
 ******************************************************************************/
#if __DARWIN_C_LEVEL >= 199506L
/*  Even though these might be more useful as long doubles, POSIX requires
    that they be double-precision literals.                                   */
...
#define M_PI        3.14159265358979323846264338327950288   /* pi             */
#define M_PI_2      1.57079632679489661923132169163975144   /* pi/2           */
...

I still have to understand that #if __DARWIN_C_LEVEL >= 199506L, but I think it is related to the C standard level (ANSI, C89, etc..)

[pattacini]

Uhm, in Windows _USE_MATH_DEFINES is said to be mandatory, regardless of the compiler's age: https://msdn.microsoft.com/en-us/library/4hwaceh6.aspx.

Yes, those constants are not in the C/C++ standard, that's true, but at least relying on cmath is way more convenient than redefining M_PI, and it seems that is available on all platforms.

[francesco-romano]

I completely agree on avoiding redefining them as if they are present on windows, then they are present also on unix :)

well.. if windows says that, I think we have to do that. And one additional define won't cause any problem on the other systems.

👍 then on exporting from the ctrlLib

[traversaro]

Do we have an automatic way in cmake to export this definition downstream, or is there any other smart solution to this problem?

The target_compile_definitions command is available since CMake 2.8.11.

An INTERFACE compile definition of a target myLib will be passed to the preprocessor when compiling any target that is linked against myLib, while a PUBLIC definition will be passed to the preprocess both when compiling myLib and when compiling any target that links myLib.

The minimum CMake version that we need to support is the 2.8.9 (see http://wiki.icub.org/wiki/YARP_Supported_Distributions) but Windows users normally have a recent version of CMake, given that they are not tied to the "system" version of CMake. I think a sensitive solution would be to add the target_compile_definitions in a IF checking both the CMake version and if the compiler is MSVC (by the way, I wonder how does CMake will deal with the recent clang support in Visual Studio 2015 Update 1 ).

Random uses of target_compile_definitions with _USE_MATH_DEFINES can be found on GitHub.

[pattacini]

Sounds great, thanks @traversaro
I'll start experimenting a bit then.

[lornat75]

I think ctrllib should not export symbols in the main namespace (i remember advising against this years ago.. :) ). modules that need those symbols should take them from cmath. Maybe for transition you could define those symbols in a specific namespace.

[drdanz]

+1 to @lornat75 reply. What's the reason for having M_PI exported? In order to do that you need to include cmath in a header of the library, is it really needed?

If you really need to do it, @traversaro reply is the way to do it with CMake 2.8.11 or later, but for CMake 2.8.9 you should also add some Xxx_DEFINITIONS variable and it becomes more complicated.
Since this define is only required on msvc, a solution could be to require different versions of CMake, i.e.

if(MSVC)
 cmake_minimum_required(VERSION 2.8.11)
else()
  cmake_minimum_required(VERSION 2.8.9)
endif()

[lornat75]

I think the problem is that there are library header files that include cmath and that the latter publicly define M_PI etc, For this reason you have to fight the order of inclusion using preprocessors flags (-D_USE_MATH_DEFINES). It would be preferable to remove inclusion of cmath from library public header files, in this way all modules could safely do:

 #define _USE_MATH_DEFINES
 #include <cmath> 

If ctrllib need to export quantities like M_PI etc it should define them as constants and hide them in the ctrllib namespace; modules that depend from ctrllib including iKin could use these symbols.

It implies some code change but in the long term it is a better solution because it removes stray preprocessor symbols that can cause clashes in user code.

[pattacini]

@lornat75 and how would you deal with for example cases like this: https://github.com/robotology/icub-main/blob/master/src/libraries/iKin/include/iCub/iKin/iKinFwd.h#L143?

[pattacini]

A possible way would be to duplicate the default constructor, or duplicate whatever other method that would need to use M_PI as default value, which seems to be a bit cumbersome.

Btw, it's true that M_PI is not a standard but there's almost an implicit convention that it's declared in cmath in all platforms. This basically makes the event of clash in user code very unlikely, since a Windows user should use in its own code cmath and at the same time redefine M_PI as well. Is there any other possibility I don't see right now?

[francesco-romano]

If I understood correctly @lornat75, I think he suggested something like:

// sort of constant.h or a main header for ctrlLib

namespace iCub {
namespace ctrl {
    extern const double pi;
}
}

// in the .cpp
#define _USE_MATH_DEFINES
#include <cmath> 

namespace iCub {
namespace ctrl {
    const double pi = M_PI;
}
}

And then use iCub::ctrl::pi in iKin, etc..

[pattacini]

I can certainly give a go to this then.

Anyway, cmath (and math.h) are somehow standard and can appear occasionally in headers, for instance those defining template types (e.g. yarp/sig/ImageDraw.h).

[lornat75]

Yes, this is what I had in mind.

I see there are still references to:

if(MSVC)
   add_definitions(-D_USE_MATH_DEFINES)
endif()

which should be no longer needed (except for compilation of ctrlLib), right?

[pattacini]

Are you referring to PR robotology/icub-main#259?

Those references are no longer needed to compile against ctrlLib, right, but some modules still use M_PI internally (they don't use the one exposed by ctrlLib), hence those modules would need that specific definition (which is required only in Windows though).

The idea behind was to achieve the following:

1. ctrlLib won't re-define M_PI.
2. ctrlLib and/or iKincan still refer to some PI constant inside their public headers to specify default values in function signatures.
3. ctrlLib will no longer include cmath in its public headers.

Then, I believe it should be more appropriate not to propagate the use of ctrlLib-like M_PI in external components, when possible, but rather rely on standard cmath for that.

What do you think?

[lornat75]

All good. In 2 you mean iKin refers to ctrl::M_PI?

I believe it should be safe to use M_PI in .c .cpp files like this:

#define _USE_MATH_DEFINES
#include <cmath>

If it works this simplifies CMake code and it makes the code more self-contained.

If it works I would always define _USE_MATH_DEFINES even in linux, it would not harm and it avoids conditional code (maybe we can add a comment to explain why we define _USE_MATH_DEFINES).

I think there is no problem if modules use ctrl::CTRL_PI unless you want to make that deprecated.

Anyway these are minor issues.

[pattacini]

Yes, iKin uses ctrl::CTRL_PI: see this line for an example of case 2.

I started off defining _USE_MATH_DEFINES inside C++ code, because I also dislike touching cmake files just for that minor issue. However, a couple of times I came across problems related to inclusion order of cmath headers (yielding quite obscure compilation errors) and then I found out that this suggestion on SE makes things clearer and prevents similar errors.

No problem for removing if(MSVC) ... endif().

[pattacini]

Condition if(MSVC) ... endif() removed from the PR.

[lornat75]

I thought order of inclusions problems would be solved by removing cmath from ctrlib public headers. strange this is not the case, maybe cmath is included by other libraries?

After reading your link it scares me that M_PI is not standard and for example is NOT defined in Linux if -std=c++11 is used (if we trust the information from one post there). In the future we could have problems. In the end it seems better if users use ctrlib::M_PI instead.

[drdanz]

Why don't we move the PI definition to yarp::math? It sounds more right than ctrlLib ...

[pattacini]

@lornat75 cmath (or math.h) is included in other headers, yes, in yarp as well, as I was saying before, eventually causing inclusion order troubles.

At the moment, ctrlLib declares its own CTRL_PI equal to M_PI; so the problem would remain, unless we switch to the use of some trigonometric computations to get PI, which need to be carried out in a sort of init function, though.

However, I'd like to point out that the use of M_PI is ubiquitous in our code (yarp, icub-main, other sister projects) and has nothing to deal with ctrlLib. Therefore, I'd rather avoid tackling this problem within this PR. I think we could conveniently merge the PR and then go on with further considerations.

@drdanz PI can be defined in yarp too, without any clash with ctrlLib, so far. Then, we can realign things later.

[lornat75]

I agree.

[pattacini]

Cool. I'd say I could keep this ticket open for future reference and in the meantime I can proceed with the PR.

[traversaro]

Hi everyone, I am a time traveler from the future. In 2020, std::numbers::pi has been standardized as part of C++20 (see https://en.cppreference.com/w/cpp/numeric/constants). As usual standard libraries will take some time to comply with the standard and be available in stable distributions, but there is hope to eventually fix this in a clean way.