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

Fix copying of data product containers #2711

Merged
merged 7 commits into from
May 1, 2024

Conversation

bocchino
Copy link
Collaborator

@bocchino bocchino commented May 1, 2024

This PR fixes an issue with copying data product containers. A DpContainer contains an Fw::ExternalSerializeBuffer as a member that provides a view into the Fw::Buffer that it owns. When a DpContainerwas copied via copy construction or copy assignment, the ExternalSerializeBuffer member was not being copied correctly.

I did the following:

  • Delete the copy assignment operator from ExternalSerializeBuffer.
  • Add two new derived classes of ExternalSerializeBuffer: ExternalSerializeBufferWithDataCopy and ExternalSerializeBufferWithMemberCopy. Each derived class adds just that kind of copy semantics to the base class.
  • Revise DpContainer so that the ExternalSerializeBuffer has type ExternalSerializeBufferWithMemberCopy. The slice is owned by the DpContainer, so this is the behavior we want.
  • Revise the tests in FppTest/dp to verify that the container assignment is working.
  • Make the copy assignment operator of SerializeBufferBase protected, so that assigning into a SerializeBufferBase reference is not allowed.
  • Revise a unit test that was assigning into a SerializeBufferBase reference, in a way that was not necessary.

@timcanham This PR pulls out the changes that I submitted to your DpCatalog branch. I thought it would be good to make a separate PR to mainline with just these changes.

@bocchino bocchino requested review from LeStarch and timcanham May 1, 2024 00:16
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Copy link
Collaborator

@timcanham timcanham left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

One minor comment. Could be fixed on follow-up.

Fw/Buffer/test/ut/TestBuffer.cpp Show resolved Hide resolved
@LeStarch LeStarch merged commit 0e1addb into nasa:devel May 1, 2024
34 checks passed
@bocchino
Copy link
Collaborator Author

bocchino commented May 1, 2024

Note: If we had getSerializeRepr return an object, instead of a reference, then we could return Fw::ExternalSerializeBufferWithMemberCopy as suggested by @LeStarch. I wrote up an issue on it here: #2714.

@bocchino bocchino deleted the fix-dp-container-copy branch June 4, 2024 20:52
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