eProsima / Fast-DDS-Gen

Fast-DDS IDL code generator tool. Looking for commercial support? Contact info@eprosima.com
Apache License 2.0
77 stars 59 forks source link

bitset implementation #211

Closed jwillemsen closed 10 months ago

jwillemsen commented 11 months ago

Is there an already existing issue for this?

Expected behavior

I am looking at the IDL to C++11 language mapping and the proposed changes for the IDL4.2 types. I notices FastDDS has some support. I do like the class implementation for the IDL bitset, but when looking at your implementation I find the (de)serialize strange. As far as I understand the bitset, when I have 15 bits used it has to be serialized as a short (16 bits), but it looks FastDDS serializes each bitfield on its own. See https://github.com/eProsima/Fast-DDS/blob/107ea8d64942102696840cd7d3e4cf93fa7a143e/test/unittest/dynamic_types/idl/new_features_4_2.cxx#L1684.

Your implementation uses a std::bitset internally, but now on each get/set bit manipulations have to be done. Maybe use a C++ struct with bitfields internally, that saves all bit manipulation during get/set, and do the bit manipulations at (de)serialize?

Current behavior

Serialize each bitfield seperately

Steps to reproduce

Code review

Fast DDS version/commit

master

Platform/Architecture

Other. Please specify in Additional context section.

Transport layer

Default configuration, UDPv4 & SHM

Additional context

Just code review

XML configuration file

No response

Relevant log output

No response

Network traffic capture

No response

JLBuenoLopez commented 11 months ago

Thanks for your contribution @jwillemsen,

I have transferred the issue to Fast DDS-Gen, as the bitset implementation is part of the generated code. I think that your issue is already being fixed as part of #208.

jwillemsen commented 10 months ago

Looking forward to check the updated generated code.