I pushed an updated version of the C++ feature detection m4 macro with changes from upstream here:
https://hg.savannah.gnu.org/hgweb/octave/rev/c8c947e1f05c

I'm still not sure how (or if) we should propagate this to user code that includes our headers.

From GCC's documentation:
https://gcc.gnu.org/onlinedocs/gcc/Standards.html

> By default, GCC also provides some additional extensions to the C++ language that on rare occasions conflict with the C++ standard. See Options Controlling C++ Dialect. Use of the -std options listed above disables these extensions where they they conflict with the C++ standard version selected. You may also select an extended version of the C++ language explicitly with -std=gnu++98 (for C++98 with GNU extensions), or -std=gnu++11 (for C++11 with GNU extensions), or -std=gnu++14 (for C++14 with GNU extensions), or -std=gnu++17 (for C++17 with GNU extensions), or -std=gnu++20 (for C++20 with GNU extensions).
>
> The default, if no C++ language dialect options are given, is -std=gnu++17.

So, I would have thought no flags would be necessary to get the full set of features for C++11 with GNU extensions with a modern GCC.
For some reason, the macro determines that it does need setting -std=gnu++11 to get what is requested...

I think the issue might be that after the patch with a compiler supporting c++17 with --enable-std-pmr-polymorphic-allocator an explicit -std=gnu++17 is supplied to the compiler, and with --disable-std-pmr-polymorphic-allocator an explicit -std=gnu+11 (disabling std::variant), whereas before this patch I suspect the -std option was not specified (and is default c++17 for gcc 11.2, allowing std::variant to be used).

@arb: Hmmm. I would have thought that that macro would prefer no flags to activate the desired feature set over any flags. I.e., I would have expected that no flags would be added for a modern GCC compiler for C++11 with GNU extensions.
I might have either misunderstood what that macro does, or GCC doesn't use GNU extensions by default.
Maybe we should remove those parts of the check and just go with whatever the compiler prefers when it comes to GNU extensions...

Setting HAVE_CXX17=0 is no signal to the compiler to disable support for C++17. It is just a means to have simpler conditions in the following tests.

What is the C++-y variant in comment #30? Is that C++20 or C++23?

Pushed a modified version of the patch from comment #25 here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9c98df2035a3

I believe we needed that change in any case.

I agree that it could be the way forward to adapt a more C++-y style of using those constant.
It looks like they are constexpr. So, that might also be something we could use.

Instead of using C constants and a gnulib module (which is mostly about C, not C++ anyway) could we check for the existence of <numbers> and provide definitions for what we need if it is missing?  Then I think we should be able to use

std::numbers::pi

and so on where we need them.  Our replacement for <numbers> might not be 100% compatible with what C++20 provides but I don't see that we need that.  All we really need is a way to access these constants in a reasonably efficient way and using the future C++ names seems better to me than using macros like M_PI.

I would generally like to be able to count on M_PI, M_E and other GNU extensions being defined.  Is there a way to do that through gnulib such that if the OS-provided header file doesn't include the definitions then the gnulib version which does is used?

However, if that is not possible then I think it would be nicer to define our own constants as a fallback rather than just aborting compilation.  I'm sure there are some weird platforms where maybe g++/gcc aren't ported and it seems mean to try and require them to build g++ from sources.

Like suggested by jwe in comment #21, I moved the implementation of the templates to a .cc file to avoid using non-ISO C/C++ in a public header file:
https://hg.savannah.gnu.org/hgweb/octave/rev/965ead41658b

Followed up by a change to reduce some duplicate code:
https://hg.savannah.gnu.org/hgweb/octave/rev/20b05c5c41a1

The last paragraph of comment #23 still stands.

@jwe: There could be other constants besides M_PI.  In data.cc, for example, wee use M_E.

#if defined (M_E)
  double e_val = M_E;
#else
  double e_val = exp (1.0);
#endif

In actual header files, though, only oct-cmplx.h uses a 'M_' constant and that constant is M_PI.

IIUC, projects including Octave headers always(?) needed to be XSI compatible anyway. (We are using things like M_PI in public headers after all.) That macro would just make that fact "more official"...

Is your intent to define _XOPEN_SOURCE in the public octave-config.h header file so that we are defining that for all users of the Octave libraries (we tell people to include octave-config.h in their .oct file sources, for example) or is this something we should only do for the Octave source files using Octave's config.h file, probably by defining something in the oct-conf-post.in.h file (stable) or oct-conf-post-public.in.h (default)?

@Rik: I searched for `octave-config.h` in all headers. In most of these, the match was in line 29. I assumed that all of those have the default license header and are including that header first thing after the inclusion guard. I manually checked the remaining headers (where the match was in a different line).

Pushed a patch here that moves "octave-config.h" to the top where it is included:
http://hg.savannah.gnu.org/hgweb/octave/rev/b7df783dbe43

I know with MACOS libraries we have had problems with just slightly different values for certain trig calculations.  By using an expression like "4.0*atan(1.0)" you get a value that is at least self-consistent with the libraries being used.  It also gets around cross-compilation issues where M_PI on the build system might be different than M_PI on the host system.  But, that would be a really bad idea if build and host were different to begin with.

In any case, I think we can just get rid of it if we can guarantee that M_PI is defined.

@markus: According to our guidelines, "octave-config.h" should always be the first header file included.  Next come C and C++ header files, then liboctave header files, then libinterp header files.

At least if it defined as "static const" then it should only be computed once (per translation unit) when Octave starts.  But then every file that needs it would have a separate (static) value.

What makes 4.0*atan(1.0) any better than just defining M_PI with a literal constant if it is not defined by including <cmath>?

Re-reading the POSIX specs:

> An XSI-conforming application shall ensure that the feature test macro _XOPEN_SOURCE is defined with the value 700 before inclusion of any header. This is needed to enable the functionality described in The _POSIX_C_SOURCE Feature Test Macro and to ensure that the XSI option is enabled.

I now think that we should define that macro in our "octave-config.h". Not just in those headers that include <cmath>...

> `atan` is not `constexpr`. But we could make that a `const double` at least.

I think it is.  I changed the code to use `constexpr` and it compiles correctly.  I believe the compiler can calculate atan at compile time by using the build system's libraries and then it inserts the correct value.

But, if you are going to handle it by using "#define" then we should define _XOPEN_SOURCE in data.cc so that we can always rely on having M_PI and then we can remove the #ifdef/#else construct entirely.

Using grep, it seems like M_PI is used more widely than just graphics.cc.  The list is

corefcn/data.cc
corefcn/gl-render.cc
corefcn/graphics.cc
corefcn/quadcc.cc
util/oct-cmplx.h
numeric/lo-mappers.cc
numeric/lo-specfun.cc
numeric/oct-norm.cc
numeric/randpoisson.cc

The correct "appropriate feature test macro" seems to be:

#define _XOPEN_SOURCE 700

See: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_01_02

It looks like this also works on Windows. And it is POSIX conform IIUC.

I'll prepare a patch.

These constants may not be part of the standard, but they are really widely available.

I don't think we need to try for perfection here.  If the Windows OS has a special exception and doesn't include M_PI then let's just figure out a way to include a definition for M_PI.

Some ideas:

1) Provide a wrapper for the the cmath header in liboctave/wrappers
2) Do a configure test for Windows and define _USE_MATH_DEFINES on that platform
3) Define M_PI ourselves if it does not exist.


On a related note, this code for the pi() function in libinterp/corefcn/data.cc is not the way to do it:

#if defined (M_PI)
  double pi_val = M_PI;
#else
  double pi_val = 4.0 * atan (1.0);
#endif

This requires calculating atan() every single time on platforms, like Windows, which don't have M_PI built-in.  At the very least the second half should be tagged with constexpr (and maybe static too).

Math constants are not defined in standard C or C++. They are a GNU/POSIX extension. See: https://en.cppreference.com/w/cpp/header/cmath

At least on Windows, compilation of headers that use any of those math constants fails if a "strict" dialect is selected (e.g. `-std=c++11`).

One header that uses e.g. M_PI is `liboctave/util/oct-cmplx.h`.

C++20 defines `std::numbers::pi` in `<numbers>`:
https://en.cppreference.com/w/cpp/numeric/constants

On Windows, the older C math constants can be forced to be defined by `<cmath>` with `#define _USE_MATH_DEFINES` prior to including this header. That also works if a "strict" C++ dialect is selected.

Should we do that? Or avoid using these non-standard constants in headers entirely? Which alternatives are there?