FFTW / fftw3

DO NOT CHECK OUT THESE FILES FROM GITHUB UNLESS YOU KNOW WHAT YOU ARE DOING. (See below.)
GNU General Public License v2.0
2.75k stars 666 forks source link

Better C++ support (#254) #255

Open maazl opened 3 years ago

maazl commented 3 years ago

The patch below allows to use fftw3.h with native support for C++ std::complex.

Since it is not possible in C++ to detect automatically whether the header <complex> has been included this is an opt in feature. The macro FFTW_cpp_complex has to be defined:

#define FFTW_cpp_complex
#include "fftw3.h"

If this is done in C++ context and the C++ version is at least C++11 then the fftw*_complex types are replaced by the class std::complex<R>.

Optionally the users may define their own implementation of FFTW_DEFINE_COMPLEX(R, C). This takes precedence over the implementation in fftw3.h. But whether this should be a documented property is questionable since wired things happen if the type is not binary compatible.

stevengj commented 2 years ago

This seems fine to me. Needs a documentation patch here: https://github.com/FFTW/fftw3/blob/7188a4c7b142c92287b025b55753959a33a2901e/doc/reference.texi#L66-L80

Just add something like "Alternatively, if you #define FFTW_cpp_complex before including fftw3.h, then FFTW will use std::complex for its complex-number types in the API. (This does not not require recompiling FFTW because of the above-mentioned binary compatibility.)"

stevengj commented 2 years ago

On a separate note, it would be good to update that section of the manual to say what version of the C++ standard eventually adopted that proposal. (2002 is no longer "recent"!)

matteo-frigo commented 1 year ago

I want to push back against this whole idea, and I say this as the dude who had the brilliant idea of redefining fftw_complex to be double _Complex if was included in a C program. (I now regard this idea as a mistake, which thankfully the world has ignored for the most part.)

The main problem with this approach is that it is not composable. (This is true both in C and C++.) A typical C++ source file in 2023 will include a thousand or so files, directly or indirectly. If any two of them have a different idea of what fftw_complex means, one change to a header file will break another unrelated header file that is included later.

The second issue is that this change does not really solve any problems. A typical C++ program will have a wrapper that converts whatever std::vector<std::complex> thingy the user may need to a call to fftw_foo(). This change only "solves" the inner part (std::complex vs. fftw_complex), but doesn't address the fact that the user really wants a vector and not a C-style array, or that the user is using the code in a template that doesn't expect to have to use different function names for every precision. A real C++ program will also want to control the lifetime of plans via constructors/destructors/move semantics/exception safety, etc. So the user has to write the wrapper anyway.

The third issue is that this change violates the C++ one-definition rule (I think).

The reality is that FFTW is a proud old-school C program and it is pointless to pretend that it is a C++ program.

Now, if somebody volunteers to develop a proper C++ API that conforms to the typical best practices of that language in 2023 so that new programs can have an easier life, I am open to having that conversation. The solution will have to look like a new header file (or something), supported by bunch of C or C++ files in a new directory api++/, and must be a purely additive contribution that does not affect existing code at all.

LecrisUT commented 1 year ago

About the C++ api implementation. It is possible to translate the macros to a combination of templates, traits and static_cast, but the only issue is that we would need to change all the way down where the macro X is defined. It might be possible to use extern, define some simple wrapper functions around it for each case and not include the C api at all, but we need to be careful to avoid compiler name (de)mangling.

stevengj commented 1 year ago

Regarding C++ apis, see also FFTW++ / github fftwpp

maazl commented 1 year ago

The main problem with this approach is that it is not composable. (This is true both in C and C++.) A typical C++ source file in 2023 will include a thousand or so files, directly or indirectly. If any two of them have a different idea of what fftw_complex means, one change to a header file will break another unrelated header file that is included later.

This argument applies to almost any #define in configuration headers.

The second issue is that this change does not really solve any problems. A typical C++ program will have a wrapper that converts whatever std::vector thingy the user may need to a call to fftw_foo(). This change only "solves" the inner part (std::complex vs. fftw_complex), but doesn't address the fact that the user really wants a vector and not a C-style array, or that the user is using the code in a template that doesn't expect to have to use different function names for every precision. A real C++ program will also want to control the lifetime of plans via constructors/destructors/move semantics/exception safety, etc. So the user has to write the wrapper anyway.

In fact I see no need to wrap the entire FFTW API (like fftw++ does).

Object lifetime can easily be managed by the standard smart pointers, e.g. std::unique_ptr<fftw_plan> with fftwf_destroy_plan as deleter. And the standard C++ containers could just use an FFTW allocator to ensure alignment. Applications with very large transforms might even allocate large blocks directly from the kernel or MMU. And if FFTW would expose the required alignment as compile time constant the C++ feature alignas might use the probably more effective allocator of the C++ runtime.

All of these options become more complicated with a full C++ wrapper that also covers the array types.

The internal storage of std::vector<T> is guaranteed to be binary compatible to a C array pointer T*. Applications with a constant plan size might even prefer a fixed size std::array. But this won't help when the type T is incompatible. A cast is required for any invocation of an API function with a complex array parameter.

The third issue is that this change violates the C++ one-definition rule (I think).

... like any other configuration #define. It is up to the programmer to ensure correct usage. Such settings should be part of the global configuration headers.

In fact a different type could be intended too. Legacy code may be partially C and C++. In this case the C parts need another type than the C++ parts.

The reality is that FFTW is a proud old-school C program and it is pointless to pretend that it is a C++ program.

Now, if somebody volunteers to develop a proper C++ API that conforms to the typical best practices of that language in 2023 so that new programs can have an easier life, I am open to having that conversation. The solution will have to look like a new header file (or something), supported by bunch of C or C++ files in a new directory api++/, and must be a purely additive contribution that does not affect existing code at all.

The basic idea of this patch is to allow a lightweight integration of FFTW into C++ code. This also simplifies the migration of old C legacy code to C++ w/o rewriting almost every line.

stevengj commented 1 year ago

This argument applies to almost any #define in configuration headers.

Only to #define statements controlled by the user, i.e. in the caller's code. (As opposed to configuration headers that are created when a library is built and are installed with the library header. But FFTW doesn't use those either.)

All of these options become more complicated with a full C++ wrapper that also covers the array types.

Yes, as C++ keeps evolving, the notion of what constitutes a "proper C++" wrapper has changed significantly over the years. But it still seems like C++ should ideally have its own header file.

matteo-frigo commented 1 year ago

@maazl Believe me, I am sympathetic to most of your points, and 20 years ago I would have happily agreed with you.

But we went down this path before, and we have come to regret it. For example, FFTW_NO_Complex exists because somebody had something like

include "foo.h"

include "fftw3.h"

where foo.h included , thus changing the meaning of fftw_complex in fftw3.h. This was in a simpler old-school world where people used dependencies sparingly, and where the C language does not encourage the needless proliferation of data types.

A typical C++ program these days has thousands of header files included multiple times. There is a whole industry of tools and so-called "best practices" devoted to "managing dependencies", with tools that reorder the inclusion order of header files depending on some set of conventions. It is expected that whatever program one writes should be usable as a subroutine of some other program.

In this world, you won't be able to write file A.h like this:

A.h:

define FFTW_cpp_complex

include

because some other file B.h, which uses the C types, may include C.h which includes A.h. So in practice people won't use the solution that you are suggesting, and we'll have wasted everybody's time documenting why they shouldn't use the feature that we provide.

maazl commented 1 year ago

@matteo-frigo Maybe another option would be to add overloads to the functions that take a pointer to std::complex* and forward the calls inlíne to the original functions. This is not intrusive and need not be part of fftw3.h at all. I did not test whether this works in all cases. The FFTW application where I got the issue is no longer affected because it switched to half complex data structures for other reasons.

leekillough commented 1 year ago

As a long-time math library and C++ expert, here's my 2 cents:

I don't think a C library like FFTW needs C++ to be added to its main code.

A separate fftw.hpp header could be added with inline and/or template definitions as wrappers, but it should not require changes to core FFTW unless absolutely necessary, and then only with #if / #ifdef guards. You must also consider which version of C++ to support -- C++98, C++03, C++11, C++14, C++17, C++20, C++23 (I would go with C++11).

_Complex was made optional in C11, after it was mandatory in C99. C++ std::complex is a representation-compatible but distinct type from C _Complex (or complex macro in <complex.h>). All of these complexities make it hard to support mixing C and C++ in a mature library like FFTW.

If you want to pass a C++ std::vector<std::complex<double>> to FFTW, you can use something like (not tested):

std::vector<std::complex<double>> vx;
fftw_complex* vx_fftw = reinterpret_cast<fftw_complex*>(&vx[0]);

... except that it technically violates strict aliasing (should not matter in practice). You could also use std::vector<fftw_complex>, and then &vx[0] would require no cast.

If you want to change FFTW, it should be in the form of the addition of a C++ header file like fftw.hpp -- no .cpp files, to avoid introducing the dependency on a C++ compiler for FFTW builds -- and it should provide wrappers to the C functions. If you want an OOP RAII model, you could even define C++ types whose constructors and destructors call the FFTW C functions to allocate and deallocate memory. inline and template should be used to avoid violating ODR.

There's also fftwpp, as mentioned earlier, as well as fftw_cpp. More examples are listed here.