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

Support sending DynamicDataAdapter sample via dynamic writer #4226

Merged
merged 16 commits into from Aug 24, 2023

Conversation

sonndinh
Copy link
Member

@sonndinh sonndinh commented Aug 8, 2023

  • Move the internal XCDR serialization functions for DynamicDataImpl out of its DataContainer. DataContainer now is just a container of the sample data which would facilitate serializing to other formats.
  • Add new interfaces for XCDR serialization to DynamicDataBase that allow dynamic writers to send dynamic samples constructed from DynamicDataAdapter in addition to DynamicDataImpl.

@sonndinh sonndinh marked this pull request as ready for review August 8, 2023 21:48
Copy link
Contributor

@jrw972 jrw972 left a comment

Choose a reason for hiding this comment

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

Keep going. We intended to move all serialization-related code out of the container and the DynamicDataImpl. The "to_xcdr" function should take a DynamicData, Serializer, and whatever else is necessary. With a free function, the same code can serialize either a DynamicDataImpl or a DynamicDataAdapter.

 @iguessthislldo 

@sonndinh
Copy link
Member Author

sonndinh commented Aug 9, 2023

Keep going. We intended to move all serialization-related code out of the container and the DynamicDataImpl. The "to_xcdr" function should take a DynamicData, Serializer, and whatever else is necessary. With a free function, the same code can serialize either a DynamicDataImpl or a DynamicDataAdapter.

 @iguessthislldo 

The serialization code for DynamicDataImpl was written to work specifically with the data container in that class. If we move it out of DynamicDataImpl, we'll need to provide a way to access the data container, e.g. via a public getter, but I think we don't want to let end user uses that interface too. Another way I can think of is making the "to_xcdr" function a friend of DynamicDataImpl. But this approach seems a bit clunky to me since many of the serialization functions called by it also need access to the data container as well.

It doesn't look like we can reuse the same serialization code of DynamicDataImpl for DynamicDataAdapter. Since the internal data of DynamicDataAdapter is a C++ value of the corresponding type, can we just use the generated operator<< from type support for it?

@sonndinh sonndinh changed the title Move XCDR serialization functions out of DynamicData container Support sending DynamicDataAdapter sample via dynamic writer Aug 22, 2023
@sonndinh sonndinh requested a review from jrw972 August 22, 2023 17:21
dds/DCPS/XTypes/DynamicDataImpl.h Outdated Show resolved Hide resolved
dds/idl/dynamic_data_adapter_generator.cpp Outdated Show resolved Hide resolved
dds/idl/dynamic_data_adapter_generator.cpp Outdated Show resolved Hide resolved
dds/DCPS/XTypes/DynamicSample.cpp Outdated Show resolved Hide resolved
dds/DCPS/XTypes/DynamicSample.cpp Outdated Show resolved Hide resolved
dds/idl/dynamic_data_adapter_generator.cpp Show resolved Hide resolved
dds/idl/dynamic_data_adapter_generator.cpp Outdated Show resolved Hide resolved
" bool serialize(OpenDDS::DCPS::Serializer& ser, OpenDDS::DCPS::Sample::Extent ext) const;\n"
"\n";

const bool is_topic_type = be_global->is_topic_type(node);
Copy link
Member

Choose a reason for hiding this comment

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

A lot of the code below is can be simplified by making full use of RefWrapper. Here's one example:

} else { // sequence, struct, union, array
RefWrapper wrapper(type, field_type_name(dynamic_cast<AST_Field*>(field), type),
prefix + "." + insert_cxx11_accessor_parens(name, is_union_member));
wrapper.nested_key_only_ = wrap_nested_key_only;
wrapper.done(&intro);
return indent + "serialized_size(encoding, size, " + wrapper.ref() + ");\n";

It will make some decisions based on the type and others based on traits set before calling done.

Copy link
Member Author

@sonndinh sonndinh Aug 23, 2023

Choose a reason for hiding this comment

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

RefWrapper is already used for types that need IDL::DistinctType. The names with _forany suffix are manually constructed because RefWrapper will generate references (I think this is needed by marshal_generator) while I need to get non-reference types to instantiate values for them. I think it's simple enough to construct those type names manually without needing to change RefWrapper.

@@ -3266,7 +3266,7 @@ bool DynamicDataImpl::move_single_to_complex(const const_single_iterator& it, Dy
}

bool DynamicDataImpl::move_single_to_complex_i(const const_single_iterator& it,
DynamicDataImpl* data, const TypeKind treat_as)
DynamicDataImpl* data, TypeKind treat_as)
Copy link
Member

Choose a reason for hiding this comment

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

The function definition can use const here, just not in the header. I think there's a note on this in the gudelines, and if not then we can add one.

Copy link
Member Author

Choose a reason for hiding this comment

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

The guidelines have a related guide (https://opendds.readthedocs.io/en/latest-release/internal/dev_guidelines.html#language-usage, point 6th) but it doesn't explicitly say when to use const.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the guidelines for this.

@jrw972 jrw972 merged commit 7c31e87 into OpenDDS:master Aug 24, 2023
118 of 124 checks passed
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.

None yet

4 participants