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.

Sign up for GitHub

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

Jump to bottom

fix: Generate correct code for nested arrays of own type #748

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

fix: Generate correct code for nested arrays of own type #748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions rosidl_generator_c/resource/full__description.c.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,16 @@ static const rosidl_type_hash_t @(c_typename)__EXPECTED_HASH = @(type_hash_to_c_

@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
@# Names for all types
@{
added_itype_names = set()
}@
@[for itype_description in all_type_descriptions]@
@[ if itype_description['type_name'] not in added_itype_names]@
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you hit a case where there was a multiple of a type in here? all_type_descriptions should be a unique set to begin with

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I ran into this problem.

//test gen rcl_interfaces/msg/ParameterValue {'type_name': 'rcl_interfaces/msg/ParameterValue', 'fields': [{'name': 'type', 'type': {'type_id': 3, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_value', 'type': {'type_id': 15, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_value', 'type': {'type_id': 8, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_value', 'type': {'type_id': 11, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_value', 'type': {'type_id': 17, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'byte_array_value', 'type': {'type_id': 160, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_array_value', 'type': {'type_id': 159, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_array_value', 'type': {'type_id': 152, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_array_value', 'type': {'type_id': 155, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_array_value', 'type': {'type_id': 161, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'value_array_value', 'type': {'type_id': 145, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': 'rcl_interfaces/msg/ParameterValue'}, 'default_value': ''}]}

static char rcl_interfaces__msg__ParameterValue__TYPE_NAME[] = "rcl_interfaces/msg/ParameterValue";
//test gen rcl_interfaces/msg/ParameterValue {'type_name': 'rcl_interfaces/msg/ParameterValue', 'fields': [{'name': 'type', 'type': {'type_id': 3, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_value', 'type': {'type_id': 15, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_value', 'type': {'type_id': 8, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_value', 'type': {'type_id': 11, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_value', 'type': {'type_id': 17, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'byte_array_value', 'type': {'type_id': 160, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_array_value', 'type': {'type_id': 159, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_array_value', 'type': {'type_id': 152, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_array_value', 'type': {'type_id': 155, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_array_value', 'type': {'type_id': 161, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'value_array_value', 'type': {'type_id': 145, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': 'rcl_interfaces/msg/ParameterValue'}, 'default_value': ''}]}

static char rcl_interfaces__msg__ParameterValue__TYPE_NAME[] = "rcl_interfaces/msg/ParameterValue";

For an unknown reason, this is not an unique set....

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I guess toplevel_msg['type_description'] and toplevel_msg['referenced_type_descriptions'] are equal in this special case ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yeah - because you have

# ParameterValue.msg
ParameterValue[] value_array_value

Then toplevel_msg['referenced_type_descriptions'] contains as one of its members the toplevel_msg['type_description']. So, there can only be this one possible case where any type gets defined twice, and that's if it is self-referencing. I think this change is fine, given that. No need to add a special-case code when you can detect it generically like this.

static char @(typename_to_c(itype_description['type_name']))__TYPE_NAME[] = "@(itype_description['type_name'])";
@{
added_itype_names.add(itype_description['type_name'])
}@
@[ end if]@
@[end for]@
@#>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

Expand Down
26 changes: 16 additions & 10 deletions rosidl_generator_c/resource/msg__struct.h.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,22 @@ enum
@[end if]@
@#>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
@# We must define the sequence type before the type itself
@# for the special case, that the type contains a sequence
@# of the type itself.
// forward declare type
struct @(idl_structure_type_to_c_typename(message.structure.namespaced_type));
// Struct for a sequence of @(idl_structure_type_to_c_typename(message.structure.namespaced_type)).
typedef struct @(idl_structure_type_sequence_to_c_typename(message.structure.namespaced_type))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I understand that you are putting the Sequence struct before the single struct, so that the single struct could contain as members a Sequence of itself, correct?

If so, maybe add that to the comment for future readers. Could even put it in a template comment @# just so it's only for people modifying the template in the future

Copy link
Author

Choose a reason for hiding this comment

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

Added a comment

{
struct @(idl_structure_type_to_c_typename(message.structure.namespaced_type)) * data;
/// The number of valid items in data
size_t size;
/// The number of allocated items in data
size_t capacity;
} @(idl_structure_type_sequence_to_c_typename(message.structure.namespaced_type));

@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
/// Struct defined in @(interface_path_to_string(interface_path)) in the package @(package_name).
@{comments = message.structure.get_comment_lines()}@
Expand Down Expand Up @@ -179,13 +195,3 @@ typedef struct @(idl_structure_type_to_c_typename(message.structure.namespaced_t
} @(idl_structure_type_to_c_typename(message.structure.namespaced_type));
@#>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
// Struct for a sequence of @(idl_structure_type_to_c_typename(message.structure.namespaced_type)).
typedef struct @(idl_structure_type_sequence_to_c_typename(message.structure.namespaced_type))
{
@(idl_structure_type_to_c_typename(message.structure.namespaced_type)) * data;
/// The number of valid items in data
size_t size;
/// The number of allocated items in data
size_t capacity;
} @(idl_structure_type_sequence_to_c_typename(message.structure.namespaced_type));
2 changes: 1 addition & 1 deletion rosidl_generator_type_description/rosidl_generator_type_description/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ def calculate_type_hash(serialized_type_description):
del field['default_value']
for referenced_td in hashable_dict['referenced_type_descriptions']:
for field in referenced_td['fields']:
del field['default_value']
field.pop('defaut_value', None)

hashable_repr = json.dumps(
hashable_dict,
Expand Down
24 changes: 20 additions & 4 deletions rosidl_typesupport_introspection_cpp/resource/msg__type_support.cpp.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ include_parts = [package_name] + list(interface_path.parents[0].parts) + [
'detail', convert_camel_case_to_lower_case_underscore(interface_path.stem)]
include_base = '/'.join(include_parts)

message_namespace = '::'.join([package_name] + list(interface_path.parents[0].parts))
message_namespaced_name = '::'.join([message_namespace, message.structure.namespaced_type.name])

header_files = [
'array',
'cstddef', # providing offsetof()
Expand All @@ -45,6 +48,19 @@ header_files = [
@[ end if]@
#include "@(header_file)"
@[end for]@


namespace rosidl_typesupport_introspection_cpp
{

// forward declare template, as it may be used in MessageMember
template<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a forward decl for a function defined lower down, right? If you could you add a comment to that effect? Is it not already declared in the .hpp.em header, and if so could we just include that instead? Why is the forward decl needed?

If we do need the forward declaration here - then I want to point out this expression

'::'.join([package_name] + list(interface_path.parents[0].parts))

Which is repeated many times in this file, and I think could be factored out into a variable at the top level block for easier reading and refactoring. Maybe factor out

message_namespace = '::'.join([package_name] + list(interface_path.parents[0].parts))
message_namespaced_name = '::'.join([message_namespace, message-structure.namespaced_type.name])

And that would really simplify some of the expressions farther down, make them more readable

Copy link
Author

Choose a reason for hiding this comment

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

We are declaring that a special instance of the template function (for the given template argument) is present, and will be defined later. If we don't do this, we will get a compile error. (Usage of template function before instantiation)

I refactored the rest of the code as suggested.

ROSIDL_TYPESUPPORT_INTROSPECTION_CPP_PUBLIC
const rosidl_message_type_support_t *
get_message_type_support_handle<@(message_namespaced_name)>();

} // namespace rosidl_typesupport_introspection_cpp

@[for ns in message.structure.namespaced_type.namespaces]@

namespace @(ns)
Expand All @@ -57,12 +73,12 @@ namespace rosidl_typesupport_introspection_cpp
void @(message.structure.namespaced_type.name)_init_function(
void * message_memory, rosidl_runtime_cpp::MessageInitialization _init)
{
new (message_memory) @('::'.join([package_name] + list(interface_path.parents[0].parts) + [message.structure.namespaced_type.name]))(_init);
new (message_memory) @(message_namespaced_name)(_init);
}

void @(message.structure.namespaced_type.name)_fini_function(void * message_memory)
{
auto typed_message = static_cast<@('::'.join([package_name] + list(interface_path.parents[0].parts) + [message.structure.namespaced_type.name])) *>(message_memory);
auto typed_message = static_cast<@(message_namespaced_name) *>(message_memory);
typed_message->~@(message.structure.namespaced_type.name)();
}

Expand Down Expand Up @@ -241,7 +257,7 @@ static const ::rosidl_typesupport_introspection_cpp::MessageMembers @(message.st
"@('::'.join([package_name] + list(interface_path.parents[0].parts)))", // message namespace
"@(message.structure.namespaced_type.name)", // message name
@(len(message.structure.members)), // number of fields
sizeof(@('::'.join([package_name] + list(interface_path.parents[0].parts) + [message.structure.namespaced_type.name]))),
sizeof(@(message_namespaced_name)),
@(message.structure.namespaced_type.name)_message_member_array, // message members
@(message.structure.namespaced_type.name)_init_function, // function to initialize message memory (memory has to be allocated)
@(message.structure.namespaced_type.name)_fini_function // function to terminate message instance (will not free memory)
Expand Down Expand Up @@ -269,7 +285,7 @@ namespace rosidl_typesupport_introspection_cpp
template<>
ROSIDL_TYPESUPPORT_INTROSPECTION_CPP_PUBLIC
const rosidl_message_type_support_t *
get_message_type_support_handle<@('::'.join([package_name] + list(interface_path.parents[0].parts) + [message.structure.namespaced_type.name]))>()
get_message_type_support_handle<@(message_namespaced_name)>()
{
return &::@('::'.join([package_name] + list(interface_path.parents[0].parts)))::rosidl_typesupport_introspection_cpp::@(message.structure.namespaced_type.name)_message_type_support_handle;
}
Expand Down