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

Correctly inform that a BoundedSequence is bounded #71

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

MiguelCompany
Copy link
Collaborator

This PR fixes a mistake I made on #67, that made bounded sequences be treated as unbounded ones.

The full fix also needs ros2/rmw_fastrtps#540, so that serialization sizes are calculated correctly.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM, pending green CI. Left a couple questions to make sure we're on the same page.

@[ else]@
size_t array_size = 0;
full_bounded = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

@MiguelCompany because it is unbounded, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct.

@@ -530,12 +530,11 @@ size_t max_serialized_size_@('__'.join([package_name] + list(interface_path.pare
size_t array_size = @(member.type.size);
@[ elif isinstance(member.type, BoundedSequence)]@
size_t array_size = @(member.type.maximum_size);
is_plain = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

@MiguelCompany because it is too early to know, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because any sequence will mean the type is not POD, and this is already covered below on the if isinstance(member.type, AbstractSequence)

@[ end if]@
@[ if isinstance(member.type, AbstractSequence)]@
full_bounded = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

@MiguelCompany because it may be abstract and bounded, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because BoundedSequence inherits from AbstractSequence and should return full_bounded = true

@hidmic
Copy link
Contributor

hidmic commented Jun 17, 2021

CI up to rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, rosidl_typesupport_fastrtps, rcl_interfaces, and test_rmw_implementation:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented Jun 22, 2021

CI again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented Aug 4, 2021

CI again as it's been a while:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic hidmic merged commit a54326a into ros2:master Aug 4, 2021
@MiguelCompany MiguelCompany deleted the bugfix/bounded-sequences branch August 5, 2021 06:59
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.

2 participants