BUG: avoid side-effect of 'include complex.h' - #26789

I wonder if we could add a test for this in the _core/tests/examples directory, which currently only builds c code tests for the limited API and cython.

Seems reasonable to add a test, probably requires only including a numpy header and then defining a variable I in a C extension. We could merge and backport this though, and leave the issue open until a test is added, in case it takes a while to write the test.

We could merge and backport this though, and leave the issue open until a test is added

Makes sense

Wouldn't this break existing uses of I in C/C++ code which may even have an explicit complex.h include?

How about adding this to our __init__.pyd instead? That way we don't get it in Cython, where it would be strange to use I, but don't break it in C/C++ where it isn't as strange and there is a decent change it is already in use.

That is especially for a backport.

Ah, sorry, I had read Cython in the title but I guess it is just as much about C code there.

Still seems a bit like we might break code that already uses complex.h, but not sure how often people actually use I from it. If that isn't super rare, it may also make sense to just document this pattern?

But, I don't have a big opinion either way, because I don't really know how likely it is we will regress others, since there seems to be no choice that regresses nobody.

I think the error this way is preferred to the (current) opposite. If we undefine I (like in this PR) and someone wants to use the macro afterward, I think the compiler error message should make it clear that I is undefined.

Any more review needed here?

No, if people notice, we may want to add docs on how to re-add I, I guess. But I guess we hope that nobody really notices anyway.

Thanks @mattip.

No, if people notice, we may want to add docs on how to re-add I, I guess. But I guess we hope that nobody really notices anyway.

Thanks @mattip.

not sure if this belongs in a new ticket, but I noticed:

/home/duncan/opt/conda/conda-bld/gstlal-inspiral_1722349839412/work/python/_spawaveform.c: In function 'SPAWaveformReduceSpin':
/home/duncan/opt/conda/conda-bld/gstlal-inspiral_1722349839412/work/python/_spawaveform.c:585:55: error: 'I' undeclared (first use in this function)
  585 |         hOfF[k] = amp * (cos(Psi+shft*f+phi0+piBy4) - I*sin(Psi+shft*f+phi0+piBy4));
      |                                                       ^

This is from a C extension module that has #include <complex.h> before #include <numpy/arrayobject.h>.

This is from a C extension module that has #include <complex.h> before #include <numpy/arrayobject.h>.

That's a bug in that code I'm afraid. The numpy headers will use #include <Python.h>, and you must include Python.h before any C standard library headers.

This is a very common bug, that only occasionally gives rise to issues (and if so, often on niche platforms/configs, like Cygwin). I'll note that SciPy has a script that's run in CI to catch the problem that may come in handy: https://github.com/scipy/scipy/blob/main/tools/check_python_h_first.py

@rgommers, thanks for following up. The header block for the relevant module looks like this:

#include <Python.h>

/* standard includes */
#include <stdio.h>
#include <math.h>
#include <complex.h>
#include <string.h>

/* LAL Includes */

#include <lal/LALDatatypes.h>
#include <lal/LALStdlib.h>
#include <lal/Units.h>
#include <lal/LALInspiral.h>

/* GSL includes */
#include <gsl/gsl_complex.h>
#include <gsl/gsl_complex_math.h>
#include <gsl/gsl_math.h>
#include <gsl/gsl_vector.h>
#include <gsl/gsl_matrix.h>
#include <gsl/gsl_linalg.h>
#include <gsl/gsl_blas.h>

#include <numpy/arrayobject.h>

As far as I can tell that follows your requirements, but ends up with error: 'I' undeclared.

This is due to issue #26787 and this PR. I am not sure what the correct path forward is, unfortunately C does not have namespaces (even though they are a honking great idea).

Ah yes, that does meet the requirement, so no bug then indeed.

No, if people notice, we may want to add docs on how to re-add I, I guess. But I guess we hope that nobody really notices anyway.

Somebody noticed .... no way to win here really.

@duncanmmacleod please open a new issue, and link to the code in question.

Hi, GCC dev here. This PR is just completely wrong.

It's one thing to insist that users must include Python.h before any other headers (I don't like it, and it doesn't compose or scale, but that problem isn't caused by numpy) but it's entirely different to include a standard header then undefine its contents so that nobody can ever use I again.

Just saying users can include <complex.h> after the numpy headers obviously doesn't work because <complex.h> has include guards so that including it a second time has no effect.

Public library headers should not be doing this.

@jwakely FWIW, we already reverted the undef.

Yeah, I'm aware it was already reverted by #27182 I just wanted to very clearly make the point that this was never OK in the first place. You can mess with the contents of standard library headers inside a .c file, but not in a public header that your downstream users consume.

As long as we have the ear of a GCC dev, #defineing the bare letter I to a value in a public header, while maybe stated in standards and whatever, is bad form, which is what got us in this quandry in the first place.

while maybe stated in standards and whatever, is bad form

GCC doesn't get to just not define something that's in the C standard and whatever.

By all means take that up with the C standard, but GCC has no choice. And you're 25+ years too late to take it up with the C standard now anyway, since it was added for C99.

Defining it for C++ (rather than only for C) is a GCC choice, not something required by the C++ standard. But is only done for -std=gnu++NN modes, not strict modes. But there should never have been a quandry, the macro is in the C standard, it's not something numpy needs to fix, and not something you can fix.