Repository navigation
Conversation
…ity. I added explanatory comments to instrument.l, instrument.y and cogen.c.in, about 300 lines across the three. The 26 removed lines are old comments I reworded; no code changed. Checking that nothing else changed: I built with devel/bin/mccode-build-conda in the mccode-dev env. The commented generator produces exactly the same C as the original for all 332 McStas example instruments, apart from the timestamp and output filename. Two side effects: The script also ran cmake --install, so mccode-dev now has a McStas built from this working tree. That differs from what was there only by these comments. I only built McStas, not McXtrace, and only on macOS. What the comments cover: Overview: how the three files fit together, and how the lexer's first-token trick lets one grammar handle both .instr and .comp files. Component loading: how .comp files are read by a nested parse started from inside a grammar rule. Lexer: its states and the stack it uses for %include files. Parser: how INHERIT/EXTEND sections are joined, the two-step component rule (including the $N numbering), and why expressions are just token strings. Generator: how the output maps onto structs, class functions and _<name>_var, plus a sketch of the raytrace() state machine. Fixes to existing comments: a few were wrong, for example the DEFINITION_TYPE options, the def_uservars header, and ".com" instead of ".comp".
Behaviour changes (may change simulation results):
- Whole-component INHERIT ("DEFINE COMPONENT X INHERIT Y") now takes
the code sections X leaves empty from Y. Before, nothing was
inherited, so Diaphragm (INHERIT Slit) and Place (INHERIT Arm) were
no-ops: Diaphragm let the full beam through (McStas and McXtrace).
Diaphragm now gives the same results as Slit.
SHARE is now emitted once per code block, so a class and the classes
inheriting it no longer duplicate it. StatisticalChopper_Monitor now
also compiles without a Monitor_nD elsewhere in the instrument.
- Literal vector parameters ({...}) are now written with full double
precision (shortest of %.15g/%.17g that reads back exactly), instead
of %g, which rounded to 6 significant digits. No shipped instrument
is affected.
- particle_getvar()/particle_getuservar_byid() now convert numeric
USERVARS to double instead of reading their memory as a double.
Arrays, pointers and structs report failure. Monitor_nD user1..user9
variables must still be declared double.
Bug fixes:
- JUMP PREVIOUS / NEXT / NEXT(n): targets are now resolved relative to
the jumping component. Before, JUMP PREVIOUS crashed the generator
and JUMP NEXT went to the first component. Unknown or out-of-range
targets are now an error (before: silently jumped to MYSELF).
- Unknown component class, or COPY of an undefined instance: proper
error instead of a segfault.
- MYSELF in a component's own parameters is now an error. Before, it
silently referred to the previous component.
- PREVIOUS in an expression with no previous component no longer leaves
an uninitialised value.
- Component USERVARS followed by EXTEND no longer drops the EXTEND block.
- DECLARE/USERVARS scanning finds every declaration on a line
("double a; double b;"), not just the first.
- %include inside C blocks accepts multi-character extensions (.hpp).
- Lexer: included file names are copied (use-after-free); a repeated
%include of an embedded library no longer counts its line twice;
indentation before %include is no longer echoed to stdout.
- Buffer overflows fixed: SEARCH SHELL paths, the DEPENDENCY/CFLAGS
buffer (new dependency_add()), literal vectors > 100 elements, the
system directory copy.
- Fixed "$s" format in the instrument parameter type error message.
- Generated code: #undef cone/polygon/polyhedron after DISPLAY, removed
a stray "// HEST!" line, _index comment shows the real index.
- Removed the duplicate "Dependency: mcstas-r.o" message.
Cleanup:
- Removed the unused Pool API (memory.c, mccode.h.in, mcformat.c.in),
parser_pool, and the mc_yyparse/mc_yyparse_component wrappers.
- One file-scope particle state table instead of four copies.
- Removed a no-op strcmp(dependency, "-lm") and unused variables.
Tested by regenerating all 332 McStas and 112 McXtrace examples against
the previous generator: no substantive output changes. Diffs were
limited to cosmetic lines and the getter casts. JUMP/SPLIT/GROUP,
user-var, StatisticalChopper, PowderN/fluorescence (xraylib) and Union
test instruments give identical results old vs new.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
Test resultsAll comparisons are between the generator built from the parent commit ( 1. Regenerating all examples
Ignored when comparing: the date and file-name header lines, the version string, the 2. Results, old vs new generator (same seed)
The McStas runs used the build after the first round of fixes. The later changes were checked on McStas by regenerating every example (section 1), not by re-running these instruments. The McXtrace runs used the final build. 3. Bugs reproduced before and after
4. Diaphragm (
|
| Flavour | Slit | Diaphragm (old) | Diaphragm (new) |
|---|---|---|---|
| McStas | 0.00246395 | 0.0401748 | 0.00246395 |
| McXtrace | 7.80158e-06 | 0.000127205 | 7.80158e-06 |
5. Vector-literal precision (%g → exact)
Test_Pol_Mirror with a modified rUpPar (1e6 rays, seed 11). Both monitors are compared; the first, 9.11031e-05, is identical in every case.
rUpPar |
Written by old generator | Old I | New I |
|---|---|---|---|
as shipped {1.0, 0.0219, 6.07, 2.0, 0.003} |
identical | 21.2727 | 21.2727 |
Qc = 0.02191234 |
0.0219123 |
21.2869 | 21.2869 |
m = 2.0000049 |
2 |
21.2727 | 21.2728 |
| all five values to 8 digits | 6 digits | 23.3289 | 23.3289 |
Shipped literals are reproduced exactly; worst case seen is 5e-6 relative.
Not tested
- Linux (gcc, clang)
- Windows (MSVC)
- macOS x86_64
- OpenACC/GPU,
-DFUNNELand MPI runs
Contributor
|
Would it be too much to ask for tests that exercise the changed behavior and bug fixes? It'd be great if I could run |
McStas and McXtrace versions of tests for ADR_20261007_GRAMMAR_FIXES: - Unittest_JUMP_RELATIVE: JUMP PREVIOUS(2) ITERATE and JUMP NEXT(2) - Unittest_INHERIT_DIAPHRAGM: whole-component INHERIT (Diaphragm vs Slit) - Unittest_USERVARS_TYPES: int USERVAR read by Monitor_nD, several declarations per line, component USERVARS+EXTEND, %include "x.hpp" - Unittest_GRAMMAR_ERRORS: *.instr.fail cases that must fail code generation (MYSELF in parameters, bad JUMP targets, COPY of undefined instance, unknown component, illegal pointer type), with a README. Expected values are in %Example lines. The old generator segfaults on or mis-handles each of them; see the README for the observed old/new behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@g5t and @mads-bertelsen new test instruments for you to look at on the McStas side will go in mcstas-comps/examples/Tests_grammar/ @farhi new test instruments for you to look at on the McXtrace side will go in mcxtrace-comps/examples/Tests_grammar |
Contributor
Author
|
Added
Expected values are in
Runs used 1e5 rays and seed 1, on macOS arm64. |
Contributor
Author
|
@g5t https://github.com/mccode-dev/McCode/actions/runs/37609263742/job/112752252161?pr=2767 gives a pretty clear indication of one issue? 😅 |
Contributor
Author
|
@g5t @farhi @mads-bertelsen another cogen-related edit is coming (added to this PR) relating to parsing of {}-lists in cogen.c |
- Vector parameters given as {...} are now split at top-level commas and
each element is written into the generated code as the C expression it
is. Before, the elements were parsed with strtod, so an expression such
as {1, 3.2*0.0219, ...} became {1, 3.2, 0, 0.0219, ...}: too many,
shifted values. That affected IN15_Vpolariser/WASP_Vpolariser
(Pol_guide_vmirror) in ILL_H5 and ILL_H5_new, which now get their
intended reflectivity parameters: intensity downstream of these
polarisers changes by factors of ~3-12 (results change, intended).
Instrument parameters can now also be used in such vectors. All other
shipped instruments generate the same values (literals are now copied
as written, e.g. "1.0" instead of "1").
- Removed the "static vectors support literal numbers ONLY" warning,
which no longer applies, and the unused copy of the old parser in
pygen.c.in.
- FUNNEL: check the particle buffer allocations, and reset livebatchsize
at the start of each batch (a SPLIT in the previous batch changes it).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Document that vector parameters keep their expressions, and the resulting change in ILL_H5/ILL_H5_new (IN15/WASP V-cavity polarisers). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
g5t
requested changes
Oct 7, 2026
g5t
left a comment
g5t
left a comment
Contributor
There was a problem hiding this comment.
I think it might be useful to allow cancelling INHERIT for specific sections without requiring a user to know that
%{
%}
means use the INHERIT'ed block while
%{
%}
means replace the INHERIT'ed section with nothing.
2 tasks done
Review of PR #2767 (g5t): for DEFINE COMPONENT X INHERIT Y the generator decided whether X provides a section by its number of lines, so an explicitly written empty block (%{ %}) counted as "not written" and Y's section was used, while the same block with a blank line inside overrode it. - Every way of writing a section (own %{..%}, INHERIT Z, EXTEND %{..%}) now marks it present (codeblock_present: linenum > 0), and the INHERIT merge tests that. Same rule as mccode-antlr (mccode-antlr#321, #325). Within a section, parts are still concatenated in order; EXTEND never pulls in the parent implicitly. - USERVARS now has the same form as the other sections and mccode-antlr: INHERIT allowed, repeated USERVARS keyword removed (unused). - Merged blocks keep their %{ line/file, so SIG_MESSAGE debug strings now show the real location; otherwise all 450 examples generate identical code. - New grammar test Unittest_INHERIT_OVERRIDE (McStas and McXtrace) and ADR update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Thanks @g5t, agreed and implemented in e9c2c8b. What was wrongFor counted as "not written", so Y's TRACE was used. The same block with a blank line inside overrode it, so whitespace decided. The rule now (same as mccode-antlr, see mccode-dev/mccode-antlr#321 / #325)
Tests (macOS arm64)
The ADR has the rule and examples. |
New child Unittest_IO_noinit writes only an empty INITIALIZE EXTEND
%{ %}: it keeps the parent's DECLARE and TRACE but not its INITIALIZE
(review of #2767). AfterNoInit_I = 0.125 (before the fix: 0.25, the
parent's INITIALIZE was used). McStas and McXtrace.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
g5t
requested changes
Oct 7, 2026
Review of #2767 (g5t): uservar_is_numeric() used strstr, so type names containing "int", "char", ... (Point, struct interaction, charge_t) counted as numeric and the generated (double) cast did not compile, while unsigned/signed alone, size_t, bool/_Bool and <stdint.h> types were reported as not readable. - A USERVAR is numeric if it is not an array or pointer and every word of its type is one of char short int long float double signed unsigned _Bool bool const volatile MCNUM size_t, or matches u?int(_least|_fast)?N_t (same rule as is_numeric_scalar in mccode-antlr). Other typedefs are not recognised (ADR updated). - particle_uservar_init() uses the same rule. It had the same substring test, and wrote p->arr[N]=0 (one past the end) for array USERVARS. char and long long USERVARS are now zeroed at start; they were uninitialised before. All 452 examples generate identical code except 10 that now also zero char/long long USERVARS; those 10 give identical detector results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
2 participants
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.
Free-form text area
Please describe what your PR is adding in terms of features or bugfixes:
Code generator review: comments, bug fixes, dead code
Commits on
cogen-improve-comments-review:instrument.l,instrument.yandcogen.c.in. There are no code changes.examples/Tests_grammar/).{...}vector parameters and adds guards to the FUNNEL code.The reasoning is in ADR_20261007_GRAMMAR_FIXES.
DEFINE COMPONENT X INHERIT Ywith no sections of its own inherited no code fromY. Diaphragm let the full beam through (about 16× the intensity behind the equivalent Slit) in both McStas and McXtrace. It now behaves like Slit.rPar={1, 3.2*0.0219, 4.07, 1, 0.003}, were parsed as plain numbers, giving{1, 3.2, 0, 0.0219, 4.07, 1, 0.003}: 7 elements, shifted and wrong.IN15_VpolariserandWASP_Vpolarisernow get the intended values. Intensity downstream of them rises by about 3 to 12 times; for example,H511_IN15_Detectorgoes from 0 to 1.6e4.H5_Iand the other detectors. The ILL%Examplevalues may need updating.Fixes
JUMP PREVIOUSno longer crashes the generator, andJUMP NEXTno longer goes to the first component. Bad JUMP targets are now an error.COPYof an undefined instance, now gives an error instead of a segfault.MYSELFin a component's own parameters is now an error, instead of silently referring to the previous component.double a; double b;).%include "x.hpp"now works inside C blocks.{...}vector parameters: each element is written into the generated code as the expression it is, so instrument parameters and function calls work. Literals are copied exactly as written, instead of being rounded to 6 digits.Cleanup
pygen.c.in.Testing
{...}vectors. Six of them differ only in formatting (e.g.1.0vs1); ILL_H5 and ILL_H5_new are described above.Unittest_JUMP_RELATIVE,Unittest_INHERIT_DIAPHRAGMandUnittest_USERVARS_TYPESpass on both flavours. The sixUnittest_GRAMMAR_ERRORS/*.instr.failcases fail with clear errors.devel/bin/mccode-build-condain the conda-forgemccode-devenv (flex/bison from conda-forge; macOS's bison 2.3 is too old). No new dependencies.Follow-up
Declaration of use of AI-tools
^ was 🤖 Generated with Claude Code and using /ponytail full
Development OS / boundary conditions
Please describe what OS you developed and tested your additions on, and if any special dependencies are required:
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 work touches the code-generator in mccode/src
My contribution contains something else