Skip to content
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

Implement C complex API in C++ #8

Closed
BatchDrake opened this issue Oct 13, 2019 · 8 comments
Closed

Implement C complex API in C++ #8

BatchDrake opened this issue Oct 13, 2019 · 8 comments
Assignees
Labels

Comments

@BatchDrake
Copy link
Owner

In systems relying on clang for compiling C++ programs (like MacOS X), the complex C API in complex.h is not exposed. This makes sigutils and suscan's headers useless for C++ programs.

In order to fix this, SU_C_* macros in <sigutils/types.h> (like SU_C_EXP or SU_C_ABS) should be conditionally defined. A candidate test predicate could be defined(__cplusplus) && defined(__APPLE__)). If this condition is met, all types and macros are defined based on the C++'s type std::complex<float>. Of course, compatibility between float _Complex and std::complex<float> must be guaranteed.

@mehdideveloper
Copy link
Collaborator

@BatchDrake I'm confused. I can compile the code without any compiler errors!
The most important change since last time is that I have updated macOS to the latest (10.15)
Here's my gcc --version output:

Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 11.0.0 (clang-1100.0.33.8)
Target: x86_64-apple-darwin19.0.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

What's your macOS version? (I have update all Macbooks I have access to, so I can't test with older versions)

@mehdideveloper
Copy link
Collaborator

Sorry my bad. Ignore the previous comment.
I'm working on the conditional definitions now (missed and misread your comment in #12)

@BatchDrake
Copy link
Owner Author

BatchDrake commented Oct 17, 2019

Don't worry!

For testing purposes in these early stages, I suggest you generate a file with a bunch of random SUCOMPLEX numbers (in other words, a regular I/Q file), read them from two programs (in C and C++, both including <sigutils/sigutils.h>), do the same math with them from both programs using the aforementioned SU_C_ macros, print the results and check whether the outputs of the C and C++ programs match. We can discuss the unit tests later.

@mehdideveloper
Copy link
Collaborator

mehdideveloper commented Oct 17, 2019

I was thinking of changing the header like this:

#if defined(__cplusplus) && defined(__APPLE__)
    #define SUCOMPLEX  std::complex<SUFLOAT>
#else
    #define SUCOMPLEX  _Complex SUFLOAT
#endif

and

#if defined(__cplusplus) && defined(__APPLE__)
    #define SU_C_REAL(c)  c.real()
    #define SU_C_IMAG(c)  c.imag()
#else
   #define SU_C_REAL(c)   creal(c)
   #define SU_C_IMAG(c)   cimag(c)
#endif

What do you think about it?
I tested it with .c files and .cpp files and with both clang and gcc-9 (in both C and C++ modes) and it worked as expected.
I'm not very advanced with C++, so I don't know if the way I used macros above meets best practices or not (I mean calling methods like c.real and c.imag)

If you agree I can test it thoroughly, change the file and send a pull request.

@BatchDrake
Copy link
Owner Author

Looks good! According to the C++ API (https://en.cppreference.com/w/cpp/numeric/complex) those methods are just fine. Moreover, .real() and .imag() are declared as constexpr, so they will be likely optimized out at compile time (which is what I expect from C's creal and cimag). Regarding those best practices you mention:

  • It is generally accepted that when you define a macro accepting parameters, you should enclose the parameters in the definition with parentheses to ensure a correct expansion. E.g:

    #define SU_C_REAL(c) (c).real()

    So that when you write something like SU_C_REAL(a + b) it expands to (a + b).real() and not to a + b.real(). We absolutely need this, because there is a bunch of inline expressions in SigDigger using these macros.

  • Since you repeat defined(__cplusplus) && defined(__APPLE__) a couple of times and somehow I suspect this test will grow in complexity for different platforms in the future, I think it could be cleaner if we just do in the beginning something like:

    #if defined(__cplusplus) && defined(__APPLE__)
    #  define SU_USE_CPP_COMPLEX_API
    #endif
    

    and test the rest of the redefinitions against SU_USE_CPP_COMPLEX_API. And if you want to get fancy, move the C and C++ definitions to different header files (something like sigutils/cdefs.h and sigutils/cppdefs.h) and conditionally include them depending on whether SU_USE_CPP_COMPLEX_API is defined or not.

@mehdideveloper
Copy link
Collaborator

mehdideveloper commented Oct 20, 2019

FYI I'm working on resolving an issue on default g++ in macOS:
It seems that I is not defined everywhere, and we have to define it in case it's not.
I can compile a simple code that imports types.h with default gcc, brew gcc-9 and brew g++-9, but not with the default g++ which complains about I.
I've tested as many compilers as I could (on macOS) and I expands to __extension__ 1.0iF in all of them.
After defining I in types.h, I get a compile error (again only in the default g++):

error: implicit conversion from '_Complex double' to 'double' is not permitted in C++
Which points to this line:

return SQ * (SU_COS(PH) + I * SU_SIN(PH));

Have you observed a similar behaviour? (just import types.h in a .cpp and see if you can compile it)
Here's my compiler info for comparison:

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 11.0.0 (clang-1100.0.33.8)
Target: x86_64-apple-darwin19.0.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@BatchDrake
Copy link
Owner Author

The problem with that definition is that it will break in g++-9.2.1 (Debian) using C++-14:

(metalloid) % g++ -finline-functions -std=c++14 example.c -o example -O1    
example.c: In function ‘int main()’:
example.c:10:25: error: unable to find numeric literal operator ‘operator""iF’
   10 | #define I __extension__ 1.0iF
      |                         ^~~~~
example.c:18:34: note: in expansion of macro ‘I’

I guess something similar will happen with the latest versions of the language. Could you share which version of the language are you using? (just print the value of __cplusplus, it expands to an integer).

Also, I see the type is double and not float. This should not be happening (SUCOMPLEX should be defined as a single-precision complex type, not to a double one).

Additionally, _Complex is not defined as a keyword in C++14, so I'm reluctant to define I this way (note the __extension__ specifier). In fact, the ultimate goal with all this is to attempt to get rid of all references to C-like _Complex expressions and replace them by std::complex ones.

Try to define SUCOMPLEX as std::complex<SUFLOAT> and I as std::complex<SUFLOAT>{0,1}. Compile issues may arise later, but they will probably be related to treating purely real expressions as complex numbers (and we can fix those cases by hand).

@mehdideveloper
Copy link
Collaborator

Could you share which version of the language are you using?

The default g++ without any compiler flags: 199711
The default g++ with std=c++14 : 201402

and I as std::complex{0,1}.

Ok this resolves the issue and now the output of the su_c_awgn macro is the same for C and C++ compiled code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants