Repository navigation
mcstas-pygen: do not crash on instrument parameters without default value - #2762
Merged
Merged
Conversation
…alue
For an instrument parameter without default value, cogen_decls in pygen.c.in
passed the NULL default_value to exp_tostring (or pygen_pyexp for string
parameters), and mcstas-pygen crashed with a segmentation fault. Such
parameters are now written without value= (instr.add_parameter('double',
'x', comment=...)), i.e. without default value in McStasScript too, like
the C code generator does (which checks isoptional).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
@tkittel thanks, this should make it to 3.9.2 that I am about to put together. (Pending succesful runs of the testsuites, currently ongoing as https://github.com/mccode-dev/McCode/actions/runs/37443619795 and https://github.com/mccode-dev/McCode/actions/runs/37443654914) |
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.
Another pygen bug, discovered while working on the new ESS instrument repo tool.
Claude:
Free-form text area
Please describe what your PR is adding in terms of features or bugfixes:
mcstas-pygencrashes with a segmentation fault on any instrument with a parameter without default value, e.g.In
cogen_decls(mccode/src/pygen.c.in), the default value of each instrument parameter was passed toexp_tostring(orpygen_pyexpfor string parameters) without checking that there is one. For a parameter without default value,default_valueis NULL. The C code generator checksisoptionalfor this (incogen.c.in).With this PR, such parameters are written without
value=, which in McStasScript means "no default value", so the translation is faithful:Parameters with a default value are written as before. A string parameter without default value is now also distinguishable from one with the default value
""(value='""').This came up in a tool for the ESS instrument repositories (mcstas-ess-instr-citool), which translates
.instrfiles withmcstas-pygento check rules on the instruments, such as that all instrument parameters have default values; that check was impossible, sincemcstas-pygencrashed on exactly the instruments breaking the rule.Test results, with
mcstas-pygenbuilt frommainand from this PR (on Linux, with the component library of McStas 3.9.1 from conda-forge):maincrashes (exit code 139); this PR writes the code shown above. With McStasScript 0.0.94,make()of the generated script gives the parameters without default value the valueNone, andwrite_full_instrument()writes them back without default value (double x,).mcstas-comps/examples: 331 give identical output withmainand this PR (ignoring date and path lines). One (Test_Dispersion_relation_TAS) fails with both, for an unrelated reason (itsSEARCH SHELLstatement runspython, which was not on the path in my test environment;mcstasfails the same way).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 crash, 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), building
mcstas-pygenwith CMake (-DBUILD_MCSTAS=ON), with bison, flex and the C compiler from conda-forge. Not tested on Windows or macOS; the change only affects the generated Python code for instrument parameters. 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
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
I have added reasoning and documentation for the change through an ADR record in our GRAMMAR section
Not done: this is a bug fix in the Python code generator (
pygen.c.in), without changes to the grammar. Happy to add one if wanted.I am attaching test output in the comments
The test results are described above (the crash before and the output after, and the comparison for all instruments in
mcstas-comps/examples).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
My contribution contains something else
🤖 Generated with Claude Code