-
Notifications
You must be signed in to change notification settings - Fork 19
XIOS handler restructure #996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Okay I added some commits that address #1000, i.e., make better use of references (to avoid unnecessary copies) and use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always happy to see more const!
The pass by value/ reference pendulum has swung a bit far in the other direction. For small arguments (e.g. fundamental types bool, int) it is generally best to pass them by value. Another area where value semantics are preferred is when assigning return values. There is a decent stack overflow post on the topic.
In a few places you could probably save a few lines by using structured bindings. However, this is just syntactic sugar.
|
Thanks @Thanduriel, these are all nice improvements! I've added them (plus a few extra similar ones) in my latest commits. |
# Have `XiosRead_test` read output of `XiosWrite_test` Fixes #990 ~~Merges into #996~~ ### Task List - [x] Defined the tests that specify a complete and functioning change (*It may help to create a [design specification & test specification](../../../wiki/Specification-Template)*) - [x] Implemented the source code change that satisfies the tests - [x] Documented the feature by providing worked example - [x] Updated the README or other documentation - [x] Completed the pre-Request checklist below --- # Test Description From the issue: > Currently, the XIOS write test checks that a file with the expected name is created, but doesn't check its contents. To facilitate this change, `XiosWrite_test` was set to be a dependency of `XiosRead_test` in `core/test/CMakeLists.txt`. As a result, if any subset of the test suite is run that includes both of them is run then the write test will come first. --- # Change Description The main code change required to support the revised testing was to set the values for the x- and y- dimensions of each domain. These aren't actually used in the model so I just set them to coincide with the local index values. I also needed to correct the names for the axis dimensions. With these changes, the `XiosWrite_test` produces output that's accepted by the `XiosRead_test`. As part of the PR, I dropped the split frequency functionality from the test with the intention of re-enabling it in a follow-up PR that addresses #898. --- # Documentation Impact None --- ### Pre-Request Checklist - [x] The requirements of this pull request are fully captured in an issue or design specification and are linked and summarised in the description of this PR - [x] No new warnings are generated - [x] Methods and Tests are commented such that they can be understood without having to obtain additional context - [x] This PR/Issue is labelled as a bug/feature/enhancement/breaking change - [x] This change conforms to the conventions described in the README
XIOS handler restructure
Fixes #850
Fixes #1000
Merges into #974This will hopefully be the last major restructuring of the XIOS handler class.
The PR includes some pruning:
enableXiosfunction.ParaGridIO_Xiosmember functions already thrown inXios.readandXios.write.configGetXmember functions.contextIdandcalendarTypearguments from the constructor because they're now set iniodef.xmlas of Move calendar, axis, and grid config to XML #993.It also includes better modularisation:
setupFilesmember function out of the constructor.parseInputFilesassetupFields.setupClient,setupContext, andsetupCalendarand have all of them called fromconfigurealongsidesetupFilesandsetupFields.Other misc changes:
constqualifier in more places.