Skip to content
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

OpenFAST Registry: allow pointers #1248

Merged
merged 6 commits into from
Sep 26, 2022
Merged

Conversation

bjonkman
Copy link
Contributor

This PR is ready to be merged.

Feature or improvement description
The Registry was modified in the SeaState code development to allow pointers (instead of allocatable arrays) in some of the data types. This PR pulls out those code changes so it can be merged into dev before #1008 is merged. As part of the registry change, error handling was also added to some auto-generated routines that had ignored errors before. After this was modified, we discovered that the error status wasn't set in the NWTC Library FreeDynamicLib routine, so that has also been fixed here.

Related issue, if one exists
#1008

Impacted areas of the software
This affects all of the types files (including NWTC Library). Code will change, but results should not.

Test results, if applicable
These should not change.

@andrew-platt
Copy link
Collaborator

@bjonkman, thanks for putting this in!

@deslaughter, this is related to what we are doing with InflowWind.

I'll try to take a look at it in the next week.

Copy link
Collaborator

@deslaughter deslaughter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo on line 496 of reg_parse.c, but everything else looks good.

@bjonkman
Copy link
Contributor Author

Good catch, @deslaughter. That typo should be fixed now. Thanks!

@rafmudaf
Copy link
Collaborator

@bjonkman #1008 notes that the Fortran pointer support breaks the restart functionality. Is that still true here?

@bjonkman
Copy link
Contributor Author

bjonkman commented Sep 21, 2022

@rafmudaf : We've addressed some of the issues that were in the original pointer code in #1008. There are still some concerns to be aware of, but it doesn't necessarily "break" restart capability. Those comments in #1008 should probably be updated.

For existing code, this modified code will behave as before. The problem is that if you use two pointers that point to the same data, on restart those pointers will be pointing to separate copies of the data. As long as the data it was pointing to is static (in the sense that modules are using its values, but not changing it), the only issue is that the restart will consume a larger memory footprint. If the developers expect one module to modify data and the other to use it, that would be a problem on restart. This isn't any different than how the mesh data structures behave with restart (and have always behaved this way). It's just important for module developers to keep in mind that the pointers have this limitation.

Adding pointers to the Registry also puts more responsibility on developers to make sure that deallocate/nullify statements are used appropriately and consistently to prevent memory leaks and/or dangling pointers. But, that's always something to watch out for when using pointers.

@rafmudaf
Copy link
Collaborator

Thanks for documenting that here @bjonkman

@rafmudaf rafmudaf added this to the v3.3.0 milestone Sep 26, 2022
@rafmudaf rafmudaf merged commit 1b4a1ac into OpenFAST:dev Sep 26, 2022
@bjonkman bjonkman deleted the f/Registry branch October 5, 2022 16:53
@rafmudaf rafmudaf mentioned this pull request Oct 27, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants