-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better C++ support (#254) #255
base: master
Are you sure you want to change the base?
Conversation
@@ -50,16 +50,22 @@ | |||
#include <stdio.h> | |||
|
|||
#ifdef __cplusplus | |||
# if __cplusplus >= 201103L && defined(FFTW_cpp_complex) | |||
# include <complex> | |||
# define FFTW_DEFINE_COMPLEX(R, C) typedef ::std::complex<R> C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naive question: What is the difference between ::std::complex
and std::complex
here?
This seems fine to me. Needs a documentation patch here: Lines 66 to 80 in 7188a4c
Just add something like "Alternatively, if you |
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"!) |
@@ -50,16 +50,22 @@ | |||
#include <stdio.h> | |||
|
|||
#ifdef __cplusplus | |||
# if __cplusplus >= 201103L && defined(FFTW_cpp_complex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add these to cmake (and autotools)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wouldn't need them for compiling FFTW itself…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point. But still they need to be included in cmake export and pkg-config. Or at least documented somewhere
Edit: This could look like:
target_compile_definitions(${fttw_libs} $<$<AND:INSTALL_INTERFACE,CMAKE_CXX_COMPILER>:FFTW_cpp_complex>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They wouldn't be in pkg-config etcetera because you would need to enable this API in manually user code — it can't be turned on automatically, since that would break backwards compatibility. Definitely would need docs, however.
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 <complex.h> 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. |
About the C++ api implementation. It is possible to translate the macros to a combination of templates, traits and |
This argument applies to almost any
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, All of these options become more complicated with a full C++ wrapper that also covers the array types. The internal storage of
... like any other configuration 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 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. |
Only to
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. |
@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" where foo.h included <complex.h>, 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: 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. |
@matteo-frigo |
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
If you want to pass a C++
... except that it technically violates strict aliasing (should not matter in practice). You could also use If you want to change FFTW, it should be in the form of the addition of a C++ header file like There's also fftwpp, as mentioned earlier, as well as fftw_cpp. More examples are listed here. |
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 macroFFTW_cpp_complex
has to be defined: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 classstd::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.