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

bitset implementation #211

Closed
1 task done
jwillemsen opened this issue Aug 14, 2023 · 2 comments · Fixed by #208
Closed
1 task done

bitset implementation #211

jwillemsen opened this issue Aug 14, 2023 · 2 comments · Fixed by #208

Comments

@jwillemsen
Copy link

Is there an already existing issue for this?

  • I have searched the existing issues

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 JLBuenoLopez transferred this issue from eProsima/Fast-DDS Aug 21, 2023
@JLBuenoLopez
Copy link
Contributor

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
Copy link
Author

Looking forward to check the updated generated code.

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 a pull request may close this issue.

2 participants