Uh oh!
There was an error while loading. Please reload this page.
Give each string representation its own buffer - #178
Merged
Conversation
StringRepresentation kept the output buffer, the nesting level and the GeoJSON ring aggregation as statics and file globals, so two representations in one process built into the same state. All of it is per-instance now. The file-scope 'outPattern' and 'domain' globals were shadowed by members of the same name in every member function, so they were dead; removed with the rest. writedOnce stays a file global on purpose. Making it per-instance is the one part of this that is observable: output() skips a representation whose domain has no front once anything has been written, so the second representation a coupled session creates would gain a first output file it does not produce today. That is a behaviour decision for the coupled runs rather than a mechanical one, and it belongs with the state work. outputstr stays public because Command::dumpString reads it directly. rungeojson.ff produces byte-identical output to dev, both files (md5 5598d4b548bd7826546815f822b01073 and 177683c7bb42b267be6baa1d98ef5acd). Refs #175
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 freeto 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.
StringRepresentationis a class you instantiate, but it kept its working state in statics and file-scope globals: the output buffer, the nesting level, and the GeoJSON ring aggregation. Two representations in one process share all of it. This makes them members.It also removes two dead globals found along the way. The file-scope
outPatternanddomainwere shadowed by members of the same name in every member function, so nothing ever read them.One part deliberately left alone
writedOncestays a file-scope global. It is the only piece here whose conversion is observable rather than mechanical:Shared, the first representation to write suppresses the first output of any other whose domain has no front yet. Per-instance, that second representation would write one file it does not write today. A coupled session creates exactly two representations (
Command.cpp:205and232), both scheduled as timetable events, so this is reachable inmasterMNHruns.Whether that extra file is wanted is a question for whoever runs the coupled cases, not something to decide inside a mechanical refactor. It is left as-is with a comment, for the state work in #175.
What this does not do
It does not fix anything that misbehaves today. The buffer is safe under sequential use —
dumpStringRepresentation()resets it before writing, as recorded in this comment on #175 — and it does not on its own make ForeFire safe to use from several threads. It removes a design smell: per-instance state that lives in a static.Verification
rungeojson.ffproduces byte-identical output todev, both files:5598d4b548bd7826546815f822b01073and177683c7bb42b267be6baa1d98ef5acd. This is the important one — the GeoJSON aggregation state is what this change restructures, andrunffdoes not cover that path.runff: KML and NetCDF both match the references within tolerance.Related: #175 (the shared-state issue), #176 (stress test), #177 (the id counter and singleton).
This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.