Do not use I as a variable name, since <complex.h> defines it as a macro - #2752
Merged
Merged
Conversation
The SasView components include <tgmath.h>, and thereby <complex.h>, which defines I as a macro for the imaginary unit. So instruments combining a SasView sample with Union_master, Refractor, Mirror_Curved_Bispectral or Mirror_Elliptic_Bispectral did not compile, since these declare a variable called I (issue mccode-dev#2546). The variable is renamed to I_in (the incident direction, next to I_reflect and I_refract) in Union_master and Refractor, and to I_root (a root of the polynomial, next to J and K) in the two mirrors. The changed files are formatted with mccode-clangformat. Using I as an identifier clashes with a system header, so it can not in general be supported by McCode; AGENTS.md now says to avoid it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Cool. Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to compilation failures in the SKADI instrument (which uses both SasView and Union), this PR avoids a temporary workaround with compilation flags that anyway did not work on all platforms. The least intrusive (and only really) solution is to stop using variables named
Iin components that might be used alongside SasView. I asked Claude to also look elsewhere forIusage, so we are getting fixes in a few contrib components as well just to be sure.Claude will
Free-form text area
Please describe what your PR is adding in terms of features or bugfixes:
This fixes #2546: instruments combining a SasView sample with
Union_master(orRefractor,Mirror_Curved_BispectralorMirror_Elliptic_Bispectral) did not compile.The cause is a clash with a system header. The SasView components include
<tgmath.h>, and thereby<complex.h>, which definesIas a macro for the imaginary unit. Every component later in the generated C file which usesIas an identifier then breaks, e.g.Coords I = coords_scale (V, 1 / v_length);inUnion_mastergiveserror: expected identifier or '(' before '__extension__'. The workaround used so far,-D__COMPLEX_H__, only works on macOS. It can also change the results of SasView models which use complex numbers, so it shouldn't be used.The fix: these components no longer use
Ias a variable name. I searched all McStas and McXtrace components and libraries for declarations ofI, and found four:Union_masterandRefractor: the localCoords I(the normalised incident direction) is renamed toI_in, next to the existingI_reflectandI_refract. The two comments with the formulas are updated accordingly.Mirror_Curved_BispectralandMirror_Elliptic_Bispectral(contrib):I, one of the rootsI,J,Kof the polynomial, is renamed toI_root.The changed files are formatted with
mccode-clangformat, which only realigns some trailing comments and wraps one line.Using
Ias an identifier can not in general be supported or recommended, since it clashes with a system header which McCode does not control. It is not possible to fix this on the SasView side instead, e.g. with#undef Iafter the include: SasView's own complex number code (cl_complex.h) usesI. SoAGENTS.md(section "Complex numbers") now says not to useIas an identifier.Test results, with McStas 3.9.0 from conda-forge on Linux, and the changed components copied next to the instruments:
SasView_barbell), a minimal Union setup withUnion_master, aRefractor, and both mirrors (attached in the comments):main: 26 compile errors (theImacro);-D__COMPLEX_H__.PSI_ICON(the only example usingRefractor) and 10 examples usingUnion_master(ILL_SALSA,Union_NCrystal_exampleand 8 tests fromTests_union) give identical output with and without this PR. The only exception is theneutroncolumn of the event list ofUnion_abs_logger_nD_scintillator, which holds uninitialised memory in any run (an unrelated bug in that component).Declaration of use of AI-tools
Please add a checkmark here if you used AI-tools during the work for this contribution
Furter, please describe how / where and for what the tools were used:
Claude Code (Anthropic, model Claude Opus 5.5) was used, under my direction, to find the cause of the compile errors, to search the components for the identifier, to write the patch, and to run the tests described above.
Development OS / boundary conditions
Please describe what OS you developed and tested your additions on, and if any special dependencies are required:
Developed and tested on Linux (Ubuntu, x86_64) with McStas 3.9.0 from conda-forge. The change only renames variables, so it should behave the same on other platforms, but it has not been tested on Windows or macOS. No new dependencies.
PR Checklist for contributing to McStas/McXtrace
For a coherent and useful contribution to McStas/McXtrace, please fill in relevant parts of the checklist:
My contribution includes patches to an existing component file
I have used the
mcdocutility and rendered a reasonable documentation page for the component (please attach as screenshot in comments!)Not done: the documentation headers are unchanged.
I have ensured that basic use of the component is OK (e.g. an instrument using it compiles?)
I have used the
mctestutility to test one or more instruments making use of the component (please attachmcviewtestreport as screenshot in comments)Not with
mctest, but by comparing the outputs of 11 example instruments with and without this PR (see above).I have used the
mccode-clangformattool to apply the standard McCode component indentation schemeI have used the
mcrun --c-lint"linter" and followed advice to remove most / all warnings that are raisedNot done: the change only renames variables.
My contribution includes patches to an existing instrument file
My contribution includes a new component file
My contribution includes a new instrument file
My work touches the code-generator in mccode/src
My work touches / adds to the runtime lib code (.c,.h etc in multiple locations
My PR is meant to fix a specific, existing issue
I have indicated the issue number here: [BUG]: MacOS Compilation Error: Name collision between <complex.h> 's I macro and Coords I variables in components ( Union_master , Refractor ) #2546
I have added documentation for the fix and possible side effects
No side effects: the results are unchanged.
AGENTS.mdnow says not to useIas an identifier.My contribution contains something else
🤖 Generated with Claude Code