Zero initialization #236

clalancette · 2017-09-06T19:52:19Z

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.

wjwwood

lgtm

As always the templates are hard to read, but there's nothing to be done about that. 😞

wjwwood · 2017-09-06T20:31:12Z

rosidl_generator_cpp/test/test_initialization.cpp

+  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


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.

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.

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.

Thanks for the heads-up. I'll run a Windows Debug job to be sure.

dirk-thomas · 2017-09-06T21:46:05Z

rosidl_generator_cpp/package.xml

@@ -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>


Please move the build dependency above the build(tool) export dependencies (around line 11).

dirk-thomas · 2017-09-06T21:49:15Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+    print('    } else if (init == rosidl_generator_cpp::MessageInitialization::DEFAULTS_ONLY) {')
+    print('      this->initialize_defaults_only();')
+    print('    }')
+    print('  }')


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.

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.

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.

mikaelarguedas · 2017-09-06T22:02:10Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+    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):


Tip: field.type.is_dynamic_array() (I know this code was just copied but I think we'll gain in readability)

dirk-thomas · 2017-09-07T16:11:27Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

-        leading_colon = ' '
-if leading_colon == ' ':
-    print()
+        init_list += ' %s(_alloc),' % (field.name)


Instead of building a string incrementally I would suggest to collect the initializations here:

init_list = [] for ...: if ...: init_list.append(field.name + '(_alloc)')

Doesn't init need to be passed to the constructor calls of each field?

Yeah, good call on both of these points. Will fix.

dirk-thomas · 2017-09-07T16:13:31Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

-@[  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)


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.

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.

dirk-thomas · 2017-09-07T16:19:37Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+                    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.


An field which is an array of primitives could still have a default value which needs to be initialized based on the MessageInitialization argument.

dirk-thomas · 2017-09-07T16:21:01Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+            # 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))


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).

dirk-thomas · 2017-09-07T16:22:31Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+@[end for]@
+  }
+
+  void zero_initialize()


To be consistent should this be named initialize_zero instead?

Yeah, definitely, will fix.

clalancette · 2017-09-07T18:16:58Z

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.

clalancette · 2017-09-07T21:06:58Z

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.

Arrays with bounded size or unbounded size don't get any special initialization done, since they store no objects at construction time.
Any field which is a non-array, primitive type gets initialized by the initialize_*() functions, controlled by the _init argument.
Arrays of primitives with fixed size get each of their elements initialized by the initialize_*() functions, controlled by the _init argument.
Arrays of non-primitives with fixed size get each of their objects initialized by the braced-initializer-list, and get _init argument passed in. The allocator versions also get the _alloc argument passed in.
As a special case, when constructing with an _alloc, _init pair, strings use a constructor initializer so we can pass in the allocator into the constructor.

Once CI is clean, I'm going to switch this back to "in review".

dirk-thomas · 2017-09-07T21:17:45Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

-        print('    %s = %s;' % (field.name, cpp_value))
-}@
+    if (_init == rosidl_generator_cpp::MessageInitialization::ALL) {
+      this->initialize_all();


Please use consistent indentation in the Python code.

dirk-thomas · 2017-09-07T21:28:39Z

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 .cpp file or if we really like to expose these functions they could additionally be declared in the header.

clalancette · 2017-09-08T00:36:18Z

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 .cpp file or if we really like to expose these functions they could additionally be declared in the header.

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 #include any of these headers has to compile the code. Moving it to a .cpp file is a potential optimization in the future. I'm going to run another CI here and see what we get.

dirk-thomas

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.

dirk-thomas · 2017-09-08T00:38:16Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+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]@


Style: space after comma.

dirk-thomas · 2017-09-08T00:42:25Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

@@ -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)


Why primitive in the function name? Doesn't it also handle initialization of arrays and potentially complex types?

dirk-thomas · 2017-09-08T00:43:08Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+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) {


Style: constant left, variable right of operator.

dirk-thomas · 2017-09-08T00:45:18Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+@[ for name,value in array_init_all]@
+    for (auto & elem : _obj->@(name)) {
+      elem = @(value);
+    }


Should this use something like std::fill instead?

Same below.

clalancette · 2017-09-08T01:36:58Z

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.

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)

dirk-thomas · 2017-09-08T01:58:59Z

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?

I agree with your conclusion that performing all the initialization in the constructor is the only option 👍

dirk-thomas · 2017-09-08T02:20:18Z

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 MessageInitialization one-to-one to the field constructor. E.g. when the actual array values are later set in the constructor body it would be more efficient to construct the vector without explicit initialization in the constructor initializer list.

clalancette · 2017-09-08T14:21:28Z

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.

clalancette · 2017-09-08T14:50:42Z

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.

clalancette · 2017-09-08T17:13:16Z

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.

Yeah, this is basically how I've been debugging this. If you look at build_isolated/rosidl_generator_cpp/rosidl_generator_cpp/rosidl_generator_cpp/msg/primitives_some_default__struct.hpp, it should have all of the cases in there (if not, we should add more test cases).

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 MessageInitialization one-to-one to the field constructor. E.g. when the actual array values are later set in the constructor body it would be more efficient to construct the vector without explicit initialization in the constructor initializer list.

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?

mikaelarguedas · 2017-09-08T17:20:46Z

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:

int8 a_value 42
int8 another_value

MsgB.msg:

MsgA msga_value {"another_value":"53"}

How will the different initialization strategy behave in that use case?

dirk-thomas · 2017-09-08T18:20:49Z

I took a closer look at the generated code. The conditional block in the constructors are not easy to read. Instead of:

if MessageInitialization::ALL
  ...
else if MessageInitialization::SKIP
  // empty
else if MessageInitialization::ZERO
  ...
else if MessageInitialization::DEFAULTS_ONLY
  ...

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):

if MessageInitialization::ALL || MessageInitialization::DEFAULTS_ONLY
  init field1 with default
  if MessageInitialization::ALL
    init field2 without default
  if MessageInitialization::ALL
    init field3 without default
  init field4 with default
elseif MessageInitialization::ZERO
  ...
else if MessageInitialization::SKIP
  // empty
else
  assert / throw

dirk-thomas · 2017-09-08T18:27:27Z

Unrelated question: I assume we also want to provide the same functionality for the C struct (e.g. MsgName_new(rosidl_runtime_c_message_initialization)). Is the goal to do that together or at a later point in time?

dirk-thomas · 2017-09-08T18:41:38Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

-leading_colon = ':'
+def array_init_list(field, args):
+    tmp = [field.type.type + args] * field.type.array_size
+    return "%s{{%s}}" % (field.name, ', '.join(tmp))


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.

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?

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.

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"?

dirk-thomas · 2017-09-08T18:47:50Z

@clalancette Regarding my previous comment about nesting: let me try to clarify.

The current implementation passes the MessageInitialization type recursively to all complex types. So in the case where the root message doesn't have default values for the complex field that seems to make sense.

But what if the root message defines a default value for something like this: OtherMsg[3] field_name "somehow describe the default value for field_name". If the user chooses to initialize "all" I am worried that the generated code will do something like this which would perform initialization multiple times:

create the array with three elements
initialize each element based on the defaults / zeros defined in the constructor of OtherMsg
overwrite the fields of all elements with the values provided by the default value for field_name

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...

clalancette · 2017-09-08T19:48:23Z

remove unnecessary NOLINT

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.

clalancette · 2017-10-17T19:47:51Z

I've added ADD_LINTER_TESTS, and I'm now clean locally. Another CI run to check:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas

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)

mikaelarguedas · 2017-10-17T22:49:45Z

rosidl_generator_cpp/package.xml

@@ -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>


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)?

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

mikaelarguedas · 2017-10-17T22:51:30Z

rosidl_generator_cpp/CMakeLists.txt

@@ -21,11 +23,14 @@ if(BUILD_TESTING)

    "msg/Empty.msg"

+    "msg/MyVector3.msg"


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)?

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?

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 👍 .

mikaelarguedas · 2017-10-17T22:55:58Z

rosidl_generator_cpp/test/test_initialization.cpp

+  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;


Nit: literals on left side of the operators (same multiple times below)

Fixed in af73047

mikaelarguedas · 2017-10-17T22:56:27Z

rosidl_generator_cpp/test/test_initialization.cpp

+      return i == "";
+    }));
+  ASSERT_EQ(2UL, def.unbounded.size());
+  ASSERT_EQ(def.unbounded[0], 0.0f);


Nit: expected value first (same multiple times below)

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>

mikaelarguedas · 2017-10-18T21:39:04Z

rosidl_generator_cpp/msg/PrimitivesSomeDefault.msg

+FieldsWithSameTypeSomeDefaults vec3
+FieldsWithSameTypeSomeDefaults[2] vec3_fixed
+FieldsWithSameTypeSomeDefaults[] vec3_unbounded
+FieldsWithSameTypeSomeDefaults[<=2] vec3_bounded


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.

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?

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

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.

mikaelarguedas · 2017-10-18T22:24:05Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

+            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))


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>

That's because it no longer contains only primitives. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-10-23T13:18:02Z

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 srv and msg are in different namespaces, so when doing the zero-fill array thing in a srv, it didn't have access to the msg symbols directly. The fix was to generate the appropriate namespace-complete symbol, which fixes the problem (I also added a test for this case). Thanks for catching that @mikaelarguedas! I think this is ready for another review now.

dirk-thomas · 2017-10-23T17:31:33Z

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.

clalancette · 2017-10-23T18:21:52Z

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 std::array<FieldsWithSameTypeSomeDefaults, 2>. Because we ended up not wanting to have huge initializer lists, those complex fields will always have their default constructor run, which will initialize all of the fields to the default values. In the case that the user passed init = ZERO, we re-initialize those fields with a zero-based object. Before the latest changes, that generated code looked like the following:

this->req.fill(FieldsWithSameTypeSomeDefaults_<ContainerAllocator>{_init});

However, this actually fails to compile. The reason is that the service is in the namespace rosidl_generator_cpp::srv, while the complex type is in the namespace rosidl_generator_cpp::msg, with no using statement. The latest code changes the above code to:

this->req.fill(rosidl_generator_cpp::msg::FieldsWithSameTypeSomeDefaults_<ContainerAllocator>{_init});

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 test_srv_initialization.cpp and add a comment explaining that this is a compile test if you think that is better. Other suggestions on how to do this compile test welcome.

mikaelarguedas · 2017-10-23T18:33:37Z

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.
If the decision is to make this a compile test only, we could get rid of the test code completely and just keep the generation of the service and corresponding Request/Response messages

clalancette added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 6, 2017

clalancette mentioned this pull request Sep 6, 2017

Tracking bug for message initialization in C++ ros2/ros2#396

Closed

3 tasks

wjwwood approved these changes Sep 6, 2017

View reviewed changes

dirk-thomas reviewed Sep 6, 2017

View reviewed changes

mikaelarguedas reviewed Sep 6, 2017

View reviewed changes

clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Sep 7, 2017

dirk-thomas reviewed Sep 7, 2017

View reviewed changes

clalancette force-pushed the zero-initialization branch from 518b5dc to 723029b Compare September 7, 2017 18:15

dirk-thomas reviewed Sep 7, 2017

View reviewed changes

dirk-thomas reviewed Sep 8, 2017

View reviewed changes

clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 17, 2017

clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 17, 2017

mikaelarguedas reviewed Oct 17, 2017

View reviewed changes

clalancette added 4 commits October 18, 2017 13:42

Rename MyVector3 -> FieldsWithSameTypeSomeDefaults

d274e99

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove the build_depend.

b6fe0bf

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>

Make sure the expected value is always first.

af73047

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove redundant default_value check.

5573a50

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

mikaelarguedas reviewed Oct 18, 2017

View reviewed changes

clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 20, 2017

clalancette added 5 commits October 20, 2017 21:10

Rename test_initialization to test_msg_initialization.

8382c55

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Use the fully-namespaced field name.

514acea

This is more correct, and makes this actually work for services that reference messages. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Make sure to check for the size of the array for fixed types.

90695d3

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add in a test for service initialization.

46c5781

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Rename PrimitivesSomeDefault -> Various.

46fdfd0

That's because it no longer contains only primitives. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 23, 2017

dirk-thomas approved these changes Oct 31, 2017

View reviewed changes

clalancette merged commit 24555d2 into master Nov 1, 2017

clalancette deleted the zero-initialization branch November 1, 2017 12:12

clalancette removed the in review Waiting for review (Kanban column) label Nov 1, 2017

clalancette mentioned this pull request Nov 2, 2017

Tracking bug for reverting changes made to ported packages for field initialization ros2/ros2#422

Closed

20 tasks

mikaelarguedas mentioned this pull request Aug 20, 2018

Ros2 revert handmade message initialization ros-perception/vision_opencv#245

Merged

		@@ -21,11 +23,14 @@ if(BUILD_TESTING)

		"msg/Empty.msg"

		"msg/MyVector3.msg"

Zero initialization #236

Zero initialization #236

Conversation

clalancette commented Sep 6, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Sep 7, 2017

clalancette commented Sep 7, 2017

Choose a reason for hiding this comment

dirk-thomas commented Sep 7, 2017

clalancette commented Sep 8, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Sep 8, 2017

dirk-thomas commented Sep 8, 2017

dirk-thomas commented Sep 8, 2017

clalancette commented Sep 8, 2017

clalancette commented Sep 8, 2017

clalancette commented Sep 8, 2017

mikaelarguedas commented Sep 8, 2017

dirk-thomas commented Sep 8, 2017

dirk-thomas commented Sep 8, 2017

dirk-thomas Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Sep 8, 2017

clalancette commented Sep 8, 2017

clalancette commented Oct 17, 2017

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Oct 23, 2017

dirk-thomas commented Oct 23, 2017

clalancette commented Oct 23, 2017

mikaelarguedas commented Oct 23, 2017

dirk-thomas Sep 6, 2017 •

edited

Loading

dirk-thomas Sep 6, 2017 •

edited

Loading

dirk-thomas Sep 8, 2017 •

edited

Loading

clalancette Oct 18, 2017 •

edited

Loading