Group message initializations by their appropriate checks

[allenh1]

I was going through message generation, and found that there were several redundant checks on message initialization.

This clears it up.

---

This change is 

[dirk-thomas]

This patch will conflict with #334. Please rebase / update this patch once the referenced PR has been merged.

[allenh1]

@dirk-thomas you got it!

[allenh1]

@dirk-thomas rebased & ready

[dirk-thomas]

The patch seems to change the initialization order of the members, no? If yes, the members should be initialized in the order they are defined in.

[allenh1]

The patch seems to change the initialization order of the members, no?

@dirk-thomas I don't think the order is changed (at least it shouldn't be). The goal here was to collapse the if checks into a single if check. To clarify what this patch does, I probably should have attached a diff for you...

Here's a diff of the generated primitives_static__struct.hpp files (where the /g/primitives_static__struct.hpp_old is before this patch is applied).

]--- /g/primitives_static__struct.hpp_old       2019-03-13 12:31:14.580896800 -0400
+++ /g/primitives_static__struct.hpp    2019-03-13 12:32:23.262555200 -0400
@@ -48,49 +48,17 @@
       rosidl_generator_cpp::MessageInitialization::ZERO == _init)
     {
       this->bool_value = false;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->byte_value = 0;
       this->char_value = 0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->float32_value = 0.0f;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->float64_value = 0.0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int8_value = 0;
       this->uint8_value = 0;
       this->int16_value = 0;
       this->uint16_value = 0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int32_value = 0l;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->uint32_value = 0ul;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int64_value = 0ll;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->uint64_value = 0ull;
     }
   }
@@ -102,49 +70,17 @@
       rosidl_generator_cpp::MessageInitialization::ZERO == _init)
     {
       this->bool_value = false;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->byte_value = 0;
       this->char_value = 0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->float32_value = 0.0f;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->float64_value = 0.0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int8_value = 0;
       this->uint8_value = 0;
       this->int16_value = 0;
       this->uint16_value = 0;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int32_value = 0l;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->uint32_value = 0ul;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->int64_value = 0ll;
-    }
-    if (rosidl_generator_cpp::MessageInitialization::ALL == _init ||
-      rosidl_generator_cpp::MessageInitialization::ZERO == _init)
-    {
       this->uint64_value = 0ull;
     }
   }

Since this is strictly removing, it does not appear to permute the order (although this is a rather trivial case).

The two originals are in a gist here.

Let me know if you want to see any specific files.

[dirk-thomas]

Can you please try it with a message file which has a mixture of fields - some with default values, some without - and share the diff for that. Thanks.

[allenh1]

Can you please try it with a message file which has a mixture of fields - some with default values, some without - and share the diff for that. Thanks.

Do you have a particular file in mind?

[dirk-thomas]

Do you have a particular file in mind?

No, not really. I would just expect that case to have a changed order.

[allenh1]

@dirk-thomas yeah, that makes sense. I just ran colcon test on the package and have some more work to do, it seems.

I looking at various__struct.hpp, the diff gets a bit ugly. Here's the two files.

I'll move this back to in progress and work on it a little later. Sorry for the noise.

[allenh1]

Okay, @dirk-thomas I got all the tests passing and updated the gist I linked to. Here's the diff for various__struct.hpp. To me, it looks like the order of initialization has remained in-tact, and the only additions were just relocated removals from above.

Could you run CI on this for me please? Apparently I can't do that anymore (unless the way to do it has changed).

[allenh1]

@mikaelarguedas you might also be a good person to look at this. Let me know if this needs any changes.

Attention was drawn to this by PVS studio, if anyone was curious.

[allenh1]

@dirk-thomas thanks for the review -- ready for another round!

[dirk-thomas]

Indentation of this block is off.

[dirk-thomas]

• Linux
• Linux-aarch64
• macOS
• Windows

[allenh1]

Okay, I'm decently sure I've got the indentation in there correctly now (sorry about that -- can be a bit confusing).

As for the CI problems, those are seemingly unrelated, as far as I can tell. I rebased on top of the current rosidl master in case that might have been the problem (though I doubt it since there was only one extra commit).

@dirk-thomas What should I look at in order to rule out these failures?

[dirk-thomas]

The failing tests on CI are unfortunately known to be flaky.

Another round after the latest updates just to be sure:

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

Thanks for the patch and for iterating on it.

[allenh1]

@dirk-thomas my pleasure, as always. 👍