-
Notifications
You must be signed in to change notification settings - Fork 125
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
Zero initialization #236
Zero initialization #236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
As always the templates are hard to read, but there's nothing to be done about that. 😞
ASSERT_EQ(1.125f, def.float32_value); | ||
ASSERT_EQ(2.4, def.float64_value); | ||
// skip checking def.uint64_value because it doesn't have a default in this | ||
// message and thus isn't initialized with DEFAULTS_ONLY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be interesting to seed the memory with a known, non-zero, pattern and then do a placement new. Then you could check to make sure uint64_value
is not 0
. Something like:
char * memory = new char[sizeof(rosidl_generator_cpp::msg::PrimitivesSomeDefault)];
ASSERT_NE(memory, nullptr);
std::memset(memory, 0xfe, sizeof(rosidl_generator_cpp::msg::PrimitivesSomeDefault));
rosidl_generator_cpp::msg::PrimitivesSomeDefault * def =
new(memory) (rosidl_generator_cpp::MessageInitialization::DEFAULTS_ONLY);
ASSERT_NE(0ULL, def->uint64_value);
// This should be in a "scope exit" to prevent a memory leak during an assert raising...
def->~PrimitivesSomeDefault();
delete[] memory;
This might be fragile, because I don't know what the C++ standard says about this, it might let a compiler initialize it to zero if it wanted to do so during the placement new. But if you wanted to try and test this case, this would be how to do it I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually a really good idea, thanks @wjwwood I've gone ahead and implemented this as you pointed out, and it seems to work as advertised, at least on Linux. I'll give it another CI job run on the other platforms to make sure that it works everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't run it now, I'd recommend running this test in a Windows debug CI job, because it's the only one that I know of that does something to the memory in order to check for uninitialized memory accesses. Though since you're doing memset before it should be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads-up. I'll run a Windows Debug job to be sure.
rosidl_generator_cpp/package.xml
Outdated
@@ -15,6 +15,8 @@ | |||
<!-- This is needed for the rosidl_message_type_support_t struct and visibility macros --> | |||
<build_export_depend>rosidl_generator_c</build_export_depend> | |||
|
|||
<build_depend>rosidl_generator_c</build_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the build dependency above the build(tool) export dependencies (around line 11).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
print(' } else if (init == rosidl_generator_cpp::MessageInitialization::DEFAULTS_ONLY) {') | ||
print(' this->initialize_defaults_only();') | ||
print(' }') | ||
print(' }') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be refactored to actually use the fact that this is a template? The extra indirection of using print
here makes the code even more difficult to read.
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on this; it wasn't clear to me that using it as a template actually made it easier to read in all cases. After looking at the implementation again, I agree with you that in the case of gen_constructor
, it makes more sense to use it as a template and not as a function with a bunch of print
statements. I'll change that. For gen_init_func
(below), I don't believe that this is the case; it is mostly control structures, with just a handful of prints embedded, and the templated version is going to be very difficult to read. For now, I'm just going to change gen_constructor
, and let's go from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For
gen_init_func
(below), I don't believe that this is the case; it is mostly control structures, with just a handful of prints embedded, and the templated version is going to be very difficult to read.
Then I would recommend to keep the control structure in a function but move the prints out of the function and do them in em. I am pretty sure the result is more readable.
for field in spec.fields: | ||
# dynamic arrays require allocator if available | ||
if field.type.is_array and \ | ||
(field.type.array_size is None or field.type.is_upper_bound): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip: field.type.is_dynamic_array()
(I know this code was just copied but I think we'll gain in readability)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
leading_colon = ' ' | ||
if leading_colon == ' ': | ||
print() | ||
init_list += ' %s(_alloc),' % (field.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of building a string incrementally I would suggest to collect the initializations here:
init_list = []
for ...:
if ...:
init_list.append(field.name + '(_alloc)')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't init
need to be passed to the constructor calls of each field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good call on both of these points. Will fix.
@[ if alloc_name == '_alloc']@ | ||
// *INDENT-ON* | ||
@[ end if]@ | ||
explicit @(spec.base_type.type)_(const ContainerAllocator & _alloc, rosidl_generator_cpp::MessageInitialization init = rosidl_generator_cpp::MessageInitialization::ALL)@(init_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will get pretty long. Together with the comment above this can be written as:
explicit ... MessageInitialization::ALL)
@[if init_list]@
: @(', '.join(init_list))
@[end if]@
If the length of that line still bothers us we could even put each initialization on a separate line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I actually think I prefer to put each on its own line so that we never run into line length problems. I'll change the code to do that.
init_strs.append('this->%s = %s;' % (field.name, cpp_value)) | ||
# Arrays are represented either by a std::vector or by a BoundedVector. | ||
# In both cases they are not primitive types, so their constructors | ||
# will automatically be run and we don't have to do anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An field which is an array of primitives could still have a default value which needs to be initialized based on the MessageInitialization
argument.
# will automatically be run and we don't have to do anything. | ||
else: | ||
if not field.type.is_array: | ||
init_strs.append('this->%s.%s();' % (field.name, funcname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default constructor of complex fields is automatically invoked. It might need to pass the MessageInitialization
argument though (probably in the initializer list above rather than in the body).
@[end for]@ | ||
} | ||
|
||
void zero_initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent should this be named initialize_zero
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely, will fix.
518b5dc
to
723029b
Compare
What I just pushed has most of the comments addressed. The two that haven't yet been addressed are @dirk-thomas really good points about complex fields and arrays. I'm still working on those, and the code may change a bunch because of that, so it is probably a good idea to hold off on reviewing until I have something more. |
All right. I think I have all of the cases done, so I'm going to run another CI. It's kind of hard to see the big picture amongst the empy control structures, and the fact that things are split in two places, so I'll spell it out below. Let me know if this logic seems correct, and if I should add this as a comment in the code somewhere.
Once CI is clean, I'm going to switch this back to "in review". |
print(' %s = %s;' % (field.name, cpp_value)) | ||
}@ | ||
if (_init == rosidl_generator_cpp::MessageInitialization::ALL) { | ||
this->initialize_all(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use consistent indentation in the Python code.
Looking at the generated code this adds three methods to each message class. Until now we didn't define any methods on message class (beside operators) in order to not restrict the namespace. I would like to keep it that way. The functionality could be moved into "private" functions within the |
As we discussed offline, for now I put a new function into the header file that does the initialization. This fixes the namespacing issue, with the downside that every file that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea that the initialization happens in two different locations. Some part in the constructor, some part in the helper function.
A user might be tempted to call the global function but would only partially what he expects.
void @(spec.base_type.type)_primitive_initializer(rosidl_generator_cpp::MessageInitialization _init, @(spec.base_type.type)_<ContainerAllocator> * _obj) | ||
{ | ||
if (_init == rosidl_generator_cpp::MessageInitialization::ALL) { | ||
@[ for name,value in init_all_strings]@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: space after comma.
@@ -219,6 +256,43 @@ for field in spec.fields: | |||
using @(spec.base_type.type) = | |||
@(cpp_full_name)<std::allocator<void>>; | |||
|
|||
@[if need_primitive_init]@ | |||
template<class ContainerAllocator> | |||
void @(spec.base_type.type)_primitive_initializer(rosidl_generator_cpp::MessageInitialization _init, @(spec.base_type.type)_<ContainerAllocator> * _obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why primitive
in the function name? Doesn't it also handle initialization of arrays and potentially complex types?
template<class ContainerAllocator> | ||
void @(spec.base_type.type)_primitive_initializer(rosidl_generator_cpp::MessageInitialization _init, @(spec.base_type.type)_<ContainerAllocator> * _obj) | ||
{ | ||
if (_init == rosidl_generator_cpp::MessageInitialization::ALL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: constant left, variable right of operator.
@[ for name,value in array_init_all]@ | ||
for (auto & elem : _obj->@(name)) { | ||
elem = @(value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use something like std::fill
instead?
Same below.
I don't love that bit of it either, but in order to be able to pass the initializer and allocator into the complex types, those must be done in the initializer list, and in order to not pollute the namespace, we need a separate function, not a method of the class (private or otherwise). The only other way I can see to meet the requirements of not polluting the namespace and initializing all of the primitive types is to do the initialization inline in the constructors. Do you have another idea on how to do it? (your other comments are easy enough to fix, I'll address those once we figure this part out) |
I agree with your conclusion that performing all the initialization in the constructor is the only option 👍 |
I guess I have to look at the generated code for a few different message definitions to see how this all fits together. With the template it is pretty difficult to imagine how this looks for a complex nested data structure. I just thought about another case to consider. If a message definition has e.g. a default value for an array field it might not be good to pass the |
The latest commits fix a bunch of bugs and most of the review comments, except for moving the initialization into the constructors. I'll do that next. |
All right. I've moved the code into the constructors. The downside to the current implementation is that there is a lot of repetition in the empy file, but I haven't come up with a good way to reduce that yet. Thoughts/advice welcome. CI is running on the latest code. |
Yeah, this is basically how I've been debugging this. If you look at
I'm not 100% sure if I understand you on this one. In the case of a bounded or unbounded vector with no defaults, no initialization is done at all (the default constructor for the std::vector/BoundedVector is called, of course, but we don't give it any parameters). In the case of a bounded or unbounded vector with defaults, we initialize the vector to have a number of elements equivalent to the number of defaults, and then we set those defaults in the body (depending on _init). In the case of a fixed-size vector, we initialize it with the correct number of elements, and then we assign those elements in the body (depending on _init). I think the only case the tests aren't currently covering is a bounded/unbounded/fixed-size array of complex types; I'll need to add a test for that, but I don't actually think we support defaults for that anyway. Does the above cover your concern, or were you thinking about something else? |
Out-of-scope question: I didnt follow the patch in detail, I just wanted to point out that we want to be able to pass default values for not primitive types in the future and we should make sure that this is extensible to allow this use case. e.g I have 2 messages: MsgA and MsgB MsgA.msg:
MsgB.msg:
How will the different initialization strategy behave in that use case? |
I took a closer look at the generated code. The conditional block in the constructors are not easy to read. Instead of:
where each section contains partially the same code maybe the following structure is easier to read and understand (I can work on that update since I suggested it if others think its a good idea):
|
Unrelated question: I assume we also want to provide the same functionality for the C struct (e.g. |
leading_colon = ':' | ||
def array_init_list(field, args): | ||
tmp = [field.type.type + args] * field.type.array_size | ||
return "%s{{%s}}" % (field.name, ', '.join(tmp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a static array of size N
this will generate the following in source code:
field_name{{SubmsgClass(_init), SubmsgClass(_init), ... <repeated N times>}}
For larger N
that is pretty extreme. Imagine someone using geometry_msgs::Vector3[1024] a_few_vectors
. In this case I don't think we can use the initialization list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I worried about this, and kicked the can down the road. Now we've reached it again :). The real problem is that std::array
element construction/initialization happens before the body of the constructor, so we can't defer it until then and make sure we pass _init
to the constructor. The other way I can think of to fix this is to use a std::shared_ptr<std::array>
instead of std::array
(and then do a make_shared
in the constructor body), but that makes accessing fixed-size arrays different from accessing everything else. Other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed, the idea we are going to try here is:
- For fixed-size arrays with small numbers of members (say, 10), do the initialization of them in the initializer list like we are currently doing.
- For fixed-sized arrays that are larger than that, let the initializer run as it normally would (i.e. let it call the default constructor of the class, with no _init argument). In the body of the constructor, we would then hand-construct objects with the appropriate _init argument, and replace the objects in the std::array with the new ones. This has a bit of a performance impact, but should generally be acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this: is that worth the effort and complexity in the template? Or should we just do 2. with the argument that for such small N the double initialization won't hurt us "significantly"?
@clalancette Regarding my previous comment about nesting: let me try to clarify. The current implementation passes the But what if the root message defines a default value for something like this:
Maybe we don't have to think too much about it since we don't support default values for complex types yet. It was just something which crossed my mind... |
I originally put in the NOLINT because cpplint was complaining about something, but it is looking like it doesn't need it anymore. uncrustify is unhappy with the indentation on that patch, though, so I removed the extra 2 spaces you added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few cosmetic comments by skimming through the code (didnt review all the logic of the generated initialization code, will try to do it tomorrow if no one gets to it by then)
rosidl_generator_cpp/package.xml
Outdated
@@ -7,6 +7,8 @@ | |||
<maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer> | |||
<license>Apache License 2.0</license> | |||
|
|||
<build_depend>rosidl_generator_c</build_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is used only for testing or as an exported dependency is it necessary to add a build_depend on it (or would the existing build_export_depend and a test_depend be enough)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, when the code for messages is generated, they all have:
#include <rosidl_generator_cpp/message_initialization.hpp>
The message_initialization.hpp
file has:
#include <rosidl_generator_c/message_initialization.h>
Thus, in order to build these messages, a dependency on rosidl_generator_c
is required. However, it is not required for building this package. So I think you are correct; we only need this as a build_export_depend
and a test_depend
. I'll make that change: b6fe0bf
rosidl_generator_cpp/CMakeLists.txt
Outdated
@@ -21,11 +23,14 @@ if(BUILD_TESTING) | |||
|
|||
"msg/Empty.msg" | |||
|
|||
"msg/MyVector3.msg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to create a new custom message type for this ? or could the feature be tested with the existing message specifications ? (in order to keep the set of messages used for testing closer to the other generators). I'm assuming this is necessary to test some specific new features, in which case an explicit name would allow to know what it's purpose is. FieldsWithSameTypeSomeDefaults
maybe (to mimic existing similar message)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is needed because I wanted to make sure we tested a "complex" type with some of the fields set to a default value, and some unset. I didn't find any other messages that had this particular semantic in rosidl_generator_cpp/msg
. I agree with you that the name would be better as something else; I'll take your suggestion and change the name to FieldsWithSameTypeSomeDefaults
(d274e99). As far as keeping the testing closer to the other generators, I would prefer to add additional tests to the other generators (including adding this message) in another PR. Does that seem reasonable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally, I meant having a message definition we could copy to the other generators packages for testing. It makes total sense not to touch the other generators in this PR but in follow-up PRs when we add the initialization features to them 👍 .
ASSERT_EQ(0ULL, def.uint64_value); | ||
ASSERT_EQ("", def.string_value); | ||
ASSERT_TRUE(std::all_of(def.float32_arr.begin(), def.float32_arr.end(), [](float i) { | ||
return i == 0.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: literals on left side of the operators (same multiple times below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in af73047
return i == ""; | ||
})); | ||
ASSERT_EQ(2UL, def.unbounded.size()); | ||
ASSERT_EQ(def.unbounded[0], 0.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: expected value first (same multiple times below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in af73047
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
It is not necessary to have rosidl_generator_c as a build_depend for this package, just as an export (and for tests). Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
FieldsWithSameTypeSomeDefaults vec3 | ||
FieldsWithSameTypeSomeDefaults[2] vec3_fixed | ||
FieldsWithSameTypeSomeDefaults[] vec3_unbounded | ||
FieldsWithSameTypeSomeDefaults[<=2] vec3_bounded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using non-primitive types so it may benefit from being renamed. Or the fields using FieldsWithSameTypeSomeDefaults
should be moved into another msg file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this particular .msg file is to put as many different things together as possible (that way, if you are debugging this stuff, there are a limited number of places to look). So I'd prefer to leave them here, unless I misunderstood your point. Despite the fact that they are now of type FieldsWithSameTypeSomeDefaults
, they are still vectors, and vec3
is short :). However, if you think I should rename them, I can do that. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah the vec3 could be renamed but it's not a big deal.
I was referring to the name of this file that is "PrimitivesSomeDefault" and this message contains non-primitive types. If we want this to be a message definition with various types (primitives and complex fields) we may want to rename it. Some other packages have a Various message definition for example. That's not a blocker, but something we may want to consider when updating the other generators, to have the same message definitions everywhere and test the same things on all generators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, I misunderstood before. Now it makes sense. I'll rename the top-level to something more descriptive after investigating the bigger issue about namespaces below.
if member.num_prealloc > 0: | ||
strlist.append('this->%s.resize(%d);' % (member.name, member.num_prealloc)) | ||
if member.zero_need_array_override: | ||
strlist.append('this->%s.fill(%s {%s});' % (member.name, member.type, fill_args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this a bit more: I have a question that I'm not sure to be able to answer just based on the design doc for interfaces.
Should I be able to use a message from the same package in a service definition without specifying the package it comes from?
Example because I know the previous sentence is not clear:
I have a package as follow:
├── CMakeLists.txt
├── msg
│ └── foo.msg
├── package.xml
└── srv
└── bar.srv
Does bar.srv have to be:
pkgA/foo[] fools
---
pkgA/foo[3] threefools
or is the following a valid definition?
foo[] fools
---
foo[3] threefools
Reading the design doc:
The base type can be one of the following:
- a primitive type from the list above: e.g. int32
- a string with an upper boundary: string<=N to limit the length of the string to N characters
- a complex type referencing another message:
- an absolute reference of a message: e.g. some_package/SomeMessage
- a relative reference of a message within the same package: e.g. OtherMessage
it seems that the answer should be "both are valid".
If both are valid, we need to update this part of the PR (and maybe others?) to use the full c++ namespace of the message and not only the type name.
this->vec3_fixed.fill(MyVector3 {_init});
-> this->vec3_fixed.fill(rosidl_generator_cpp::msg::MyVector3 {_init});
If only the definition using "absolute references" is allowed, we should update the design document to make it clearer.
Example branch that fails to compile with the current patch: https://github.com/ros2/rcl_interfaces/tree/more_services
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This is more correct, and makes this actually work for services that reference messages. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
That's because it no longer contains only primitives. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
It turned out that the problem that @mikaelarguedas was talking about wasn't really a design problem so much as a bug in my implementation. The problem was that |
Why do the services need a separate initialization test? Each part of a service is just a message so their initialization should be covered by the message tests already. |
The service test is to ensure that the generator correctly generates fully-namespaced objects when re-initializing zero-filled objects. This is a bit dense, so let me show an example based on what Mikael talked about earlier. Let's assume you have a service with a fixed-size array of a complex type for the request (like https://github.com/ros2/rosidl/pull/236/files#diff-690610169303fba9f4575f67fb3590a9). The generated C++ type for that field will be
However, this actually fails to compile. The reason is that the service is in the namespace
And the new test is there to ensure that the rosidl generator is generating the correct code. To be honest, this test is mostly a compile-test; if the compilation succeeds, we've generated the correct thing, and we don't need to go much further than that. However, I thought it wouldn't hurt much to add a couple of checks in there as well. I can remove the bulk of the code from |
I don't feel strongly about testing the content or not given that the initialization itself is the same as for messages, but as you said I can't hurt. |
This PR implements the different forms of initialization that we want to allow for the instantiation of messages, as discussed in ros2/ros2#396 and ros2/design#136 . The default is to do full initialization of primitive message fields with the default values (where those exist) or zeros (for everything else). Complex types (such as
std::string
,std::array
, etc) already have their constructors called, so there is no need to do anything specific there.connects to ros2/ros2#396
CI is also in the above linked issue.