Skip to content
This repository has been archived by the owner on Oct 23, 2020. It is now read-only.

Update registry parser to validate nested structs #453

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

douglasjacobsen
Copy link
Member

This merge updates the validator within the registry parser to properly
validate nested var_structs. Previously, nested var_structs were ignored
entirely by the validator.

This commit updates the validator within the registry parser to properly
validate nested var_structs. Previously, nested var_structs were ignored
entirely by the validator.
@douglasjacobsen
Copy link
Member Author

For reference, you can define nested var_structs in Registry.xml as follows:

    <var_struct name="testStruct" time_levs="1">
        <var name="testField" dimensions="nCells Time" type="real"/>
        <var_struct name="testNestStruct" time_levs="1">
            <var name="testField2" dimensions="nCells Time" type="real"/>
        </var_sruct>
    </var_struct>

This is an example we would expect to pass, because each one has a unique name. Previously nothing was validated under the top level struct, so something like the following, would have passed but should have failed:

    <var_struct name="testStruct" time_levs="1">
        <var name="testField" dimensions="nCells Time" type="real"/>
        <var_struct name="testStruct" time_levs="1">
            <var name="testField2" dimensions="nCells Time" type="real"/>
        </var_sruct>
    </var_struct>

or

    <var_struct name="testStruct" time_levs="1">
        <var name="testField" dimensions="nCells Time" type="real"/>
        <var_struct name="testNestStruct" time_levs="1">
            <var name="testField" dimensions="nCells Time" type="real"/>
        </var_sruct>
    </var_struct>

@mgduda
Copy link
Contributor

mgduda commented Jul 1, 2015

@douglasjacobsen Is there a particular reason for keeping the commented-out code in parse.c between lines 805 and 1100? If this code is not needed, but is being left as a comment for posterity, we could add an additional commit that remove the code, thereby maintaining a commit (a162e3f) in the repository where the code could always be found.

@douglasjacobsen
Copy link
Member Author

@mgduda You're right, I should have removed those fields.

I'm not 100% sure this is ready to merge though, I think some additional questions need to be answered, like what we should do with nested structs that share the same name and should be merged. Does validation happen correctly on these for example?

Anyway, this is mostly just a starting place to get to where validation of these complicated registry files is a bit easier.

@mgduda
Copy link
Contributor

mgduda commented Jul 1, 2015

In some initial testing, I added the following to the Registry.xml file for the sw core:

      <var_struct name="testStruct" time_levs="1">
              <var name="testField" dimensions="nCells Time" type="real"/>
              <var_struct name="testNestStruct" time_levs="1">
                      <var name="testField2" dimensions="nCells Time" type="real"/>
              </var_sruct>
      </var_struct>

After making this addition, the build fails with the following error message:

Reading registry file from standard input
ERROR: Trying to add undefined variable u to stream input.
Validation failed.....

@mgduda
Copy link
Contributor

mgduda commented Jul 1, 2015

@douglasjacobsen As a side note, perhaps we could create a new tag for pull requests that aren't ready to be merged? There's a real danger that a reviewer may not be aware that a PR isn't ready, and might merge the PR after some not-so-thorough testing.

@douglasjacobsen
Copy link
Member Author

@mgduda Sure, I've added a "Don't merge" label to the PR.

As far as your error goes, we've seen that error before in the ocean core without the modifications in this PR, so I don't think this changes that behavior at all. I haven't investigated what is causing that issue though. In our cases, the fix was to move the stream definition below the variable definition, but I don't see any reason why that should be a requirement.

@mark-petersen
Copy link
Contributor

I will rebase and test in ocean core.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants