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

Move Ssc-internal struct manipulation to Ssc-Internal Maps #3406

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

eisenhauer
Copy link
Member

This is a step towards a broader rework of struct handling. (Jason, review if you can, but this is mostly moving the structure map internal so that I don't break Ssc with future changes.

@eisenhauer eisenhauer merged commit 443318b into ornladios:master Dec 12, 2022
@eisenhauer eisenhauer deleted the SscStructMove branch December 12, 2022 23:34
Copy link
Contributor

@anagainaru anagainaru left a comment

Choose a reason for hiding this comment

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

I am a little confused why you need to do this, but I assume you have a reason, so my only review is to make sure you chance all the places affected by this (in case we don't test all corner cases). Looks good as far as I can tell.

There is a DefineStruct API defined in the IO object in the cpp bindings which won't have a correspondent in core/IO. Is this also going to change?

@eisenhauer
Copy link
Member Author

eisenhauer commented Dec 13, 2022

@anagainaru Sorry, should have waited for you. The bigger goal is to revamp the Struct API before we finalize it. The most significant change will be to read-side semantics where I'd like to move away from the requirement that struct names be unique in an ADIOS, and really try to move away from a name-based semantics in the struct API completely. (For example, struct names are superfluous on the writer side already, yet the implementation imposes an unnecessary uniqueness requirement.) Most of these changes will affect BP5-based engines, and I was preparing a bigger PR, but those changes were getting tangled up with attempts to keep existing Ssc functionality from breaking. I thought it would be more straightforward to do this piecewise, starting with trying to move things that were more properly ssc-internal out of the way a bit.

@JasonRuonanWang
Copy link
Member

I am a little confused why you need to do this, but I assume you have a reason, so my only review is to make sure you chance all the places affected by this (in case we don't test all corner cases). Looks good as far as I can tell.

There is a DefineStruct API defined in the IO object in the cpp bindings which won't have a correspondent in core/IO. Is this also going to change?

For this kind of changes, you either break everything, or you break nothing. I believe Greg completely knows what he does. Besides, in software engineering, when you want to do some large refactoring work, the best practice is to decouple complicated logic dependencies first so that when you start refactoring, you can do it a little step by a little step, with all the tests passing throughout the entire process. So from my stand point, what Greg's doing is completely and easily understandable.

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

Successfully merging this pull request may close these issues.

3 participants