Clean up Magic Numbers #140

allenh1 · 2017-08-10T18:26:23Z

This is still in progress, but opening this for visibility.

connects to #72

dirk-thomas · 2017-08-10T18:41:10Z

rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.hpp

@@ -732,30 +733,34 @@ size_t TypeSupport<MembersType>::calculateMaxSerializedSize(
        case ::rosidl_typesupport_introspection_cpp::ROS_TYPE_UINT8:
        case ::rosidl_typesupport_introspection_cpp::ROS_TYPE_CHAR:
        case ::rosidl_typesupport_introspection_cpp::ROS_TYPE_INT8:
-          current_alignment += 1;
+          current_alignment += sizeof(int8_t);


Are those actually dependent on the system or is the alignment coming from the DDS spec? In the latter case the literal number makes more sense imo.

Same below.

That's a good question, and one I do not know the answer to. Perhaps it would be ok to say this?

current_alignment += 1; /* sizeof(int8_t) */

@allenh1 Did we come to a final answer on this ? should this use sizeof or should this be kept a hardcoded value based on the specs ?

I think we agreed upon the sizeof, but I'll defer the call, as I am no expert.

I didnt look at the spec. If you checked both specifications (DDS and RTPS) and that nothing specifies the padding I'm fine with merging this as is

In DDS, the only statement about padding was dealing with the required 4 bytes. I'm not sure about RTPS, I'll have to get back to you.

allenh1 · 2017-08-10T18:51:33Z

@mikaelarguedas I just tested this branch (locally) with your new tests, and all seems to be ok.

Are those actually dependent on the system or is the alignment coming from the DDS spec?

Do you know the answer to this question? I don't know enough about DDS to make a claim there.

mikaelarguedas · 2017-08-10T19:03:18Z

mikaelarguedas I just tested this branch (locally) with your new tests, and all seems to be ok.

cool!

Do you know the answer to this question? I don't know enough about DDS to make a claim there.

No I don't, if it's in the spec I would expect it to be in the RTPS spec.
If we end up being able to use if it's not in the spec, would it make more sense to use alignof rather than sizeof here ?

I guess the other unknown is what we can do about the space variable that is set to 100

allenh1 · 2017-08-10T19:07:40Z

If we end up being able to use if it's not in the spec, would it make more sense to use alignof rather than sizeof here ?

Maybe. Is that preferred?

I guess the other unknown is what we can do about the space variable that is set to 100

I totally spaced on that one. I'll fix that.

mikaelarguedas · 2017-08-11T15:47:36Z

If we end up being able to use if it's not in the spec, would it make more sense to use alignof rather than sizeof here ?

Maybe. Is that preferred?

TL;DR I think that if the current_alignment variable represents a size we should use sizeof, if it represent an alignment we should use alignof. In this specific case we use it on types where the 2 functions will return the same value so it doesn't matter that much. (rambling below)

If the alignment if not specified in the spec, I think we should use alignof to find the alignment. Though for primitive types like the ones used here it should give the same result as sizeof. But it would be more semantically correct and people wouldnt assume they can call sizeof on more complicated structures (where the result will differ quite a lot from alignof).

Though I didn't look at how this current_alignment is used, it would be valuable to know if it is supposed to represent an actual alignment or a size (because calculateMaxSerializedSize returns arguably a size that is added to the current_alignment). According to what it represents we can chose the appropriate function to use.

allenh1 · 2017-08-11T15:55:30Z

If the alignment if not specified in the spec, I think we should use alignof to find the alignment. Though for primitive types like the ones used here it should give the same result as sizeof. But it would be more semantically correct and people wouldnt assume they can call sizeof on more complicated structures (where the result will differ quite a lot from alignof).

I think you're right. Unfortunately, we just don't know what it is supposed to be. So, I think the best idea is to change it to alignof (since it makes the most sense to me), then spin up a CI job. If all is green on arm64, most of my concerns will disappear.

mikaelarguedas · 2017-08-11T15:58:15Z

I would check the RTPS and CDR specs first though as suggested in #140 (comment)

allenh1 · 2017-08-11T16:01:53Z

I would check the RTPS and CDR specs first though as suggested in #140 (comment)

Ok, sounds like a plan.

allenh1 · 2017-08-13T19:34:35Z

CI (updated after f9dcf15)

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2017-08-14T16:43:17Z

rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.hpp

            void * subros_message = nullptr;
            size_t array_size = 0;
            size_t sub_members_size = sub_members->size_of_;
            size_t max_align = calculateMaxAlign(sub_members);
-            size_t space = 100;
+            size_t space = 1;  /* start with 1 byte */


Can you please clarify why 1 is being used here? The comment doesn't provide much information.

The comment should also be // instead of /* ... */.

I picked 1 since everything in the system is based on octets, so that's the smallest unit of space one can have. That's the most sensible value I could come up with -- does this makes sense? I'm not very confident with it at 1, but I think it's better than having it set to 100.

As far as I can see the variable is being used as an "upper limit" on how much alignment can accumulate. I don't see why 1 would make sense for this?

Btw. there is a second location where that constant is being used:

rmw_fastrtps/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.hpp

Line 684 in af079ef

size_t space = 100;

As far as I can see the variable is being used as an "upper limit" on how much alignment can accumulate. I don't see why 1 would make sense for this?

In that case, the value should be 4. I'll update it as such.

Btw. there is a second location where that constant is being used.

I'll update that as well. Thanks.

In that case, the value should be 4. I'll update it as such.

Why?

If I'm understanding how DDS serialization works (which is a big if), that's how something of size 0 would be aligned.

If I'm understanding how DDS serialization works (which is a big if), that's how something of size 0 would be aligned.

As far as I read the code this is not how this variable is being used.

mikaelarguedas · 2017-08-23T17:40:06Z

@allenh1 any update on this ?

…variable, encapsulation, to keep track of the magic number four added in several places.

mikaelarguedas · 2017-08-30T20:43:36Z

@allenh1 If there is no obvious path forward here (figuring out if we can get rid of space completely or a default that makes more sense than the current one), please change (or split) this PR so that the removal of the other magic numbers can be merged while we decide what to do with the space variable

allenh1 · 2017-08-31T20:08:44Z

@mikaelarguedas sounds good to me

allenh1 · 2017-08-31T20:16:27Z

CI:

Linux
Linux-aarch64
macOS
Windows

It appears windows failed with the connext middleware, so I don't think this failure is related to the PR.

mikaelarguedas · 2017-08-31T23:21:52Z

It appears windows failed with the connext middleware, so I don't think this failure is related to the PR.

Yes the failure is unrelated, this is a test known to be flaky on windows.

Can you please address / answer the comments ?

allenh1 · 2017-09-01T21:47:38Z

@mikaelarguedas Ok, after looking at the RTPS spec, things are still fine (the sizes are recorded within the header).

Good to merge?

mikaelarguedas · 2017-09-01T21:49:26Z

👍 please squash and merge ;)

allenh1 added the in progress Actively being worked on (Kanban column) label Aug 10, 2017

dirk-thomas reviewed Aug 10, 2017

View reviewed changes

allenh1 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 13, 2017

allenh1 force-pushed the remove-magic-numbers-72 branch from 0df786c to f9dcf15 Compare August 13, 2017 21:31

dirk-thomas reviewed Aug 14, 2017

View reviewed changes

allenh1 added 5 commits August 29, 2017 21:37

Replace hardcoded size of types with a sizeof call, as well as add a …

78d06ce

…variable, encapsulation, to keep track of the magic number four added in several places.

Remove the max string length and max array size.

b628690

Rename encapsulation variable.

3e0e43f

Passing tests

d2125fd

whitespace

cb88ff3

allenh1 force-pushed the remove-magic-numbers-72 branch from f9dcf15 to cb88ff3 Compare August 30, 2017 04:37

Revert space variable change.

27e85f2

allenh1 force-pushed the remove-magic-numbers-72 branch from 8c7d1a1 to 27e85f2 Compare August 31, 2017 20:14

allenh1 merged commit 79d8dc4 into master Sep 1, 2017

allenh1 deleted the remove-magic-numbers-72 branch September 1, 2017 21:58

allenh1 removed the in review Waiting for review (Kanban column) label Sep 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Magic Numbers #140

Clean up Magic Numbers #140

allenh1 commented Aug 10, 2017

dirk-thomas Aug 10, 2017

allenh1 Aug 10, 2017

mikaelarguedas Aug 31, 2017

allenh1 Sep 1, 2017

mikaelarguedas Sep 1, 2017

allenh1 Sep 1, 2017

allenh1 commented Aug 10, 2017

mikaelarguedas commented Aug 10, 2017

allenh1 commented Aug 10, 2017

mikaelarguedas commented Aug 11, 2017

allenh1 commented Aug 11, 2017

mikaelarguedas commented Aug 11, 2017

allenh1 commented Aug 11, 2017

allenh1 commented Aug 13, 2017 •

edited

Loading

dirk-thomas Aug 14, 2017

allenh1 Aug 14, 2017

dirk-thomas Aug 14, 2017

dirk-thomas Aug 14, 2017

allenh1 Aug 14, 2017

dirk-thomas Aug 14, 2017

allenh1 Aug 14, 2017

dirk-thomas Aug 14, 2017

mikaelarguedas commented Aug 23, 2017

mikaelarguedas commented Aug 30, 2017

allenh1 commented Aug 31, 2017

allenh1 commented Aug 31, 2017 •

edited

Loading

mikaelarguedas commented Aug 31, 2017

allenh1 commented Sep 1, 2017

mikaelarguedas commented Sep 1, 2017

Clean up Magic Numbers #140

Clean up Magic Numbers #140

Conversation

allenh1 commented Aug 10, 2017

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

allenh1 commented Aug 10, 2017

mikaelarguedas commented Aug 10, 2017

allenh1 commented Aug 10, 2017

mikaelarguedas commented Aug 11, 2017

allenh1 commented Aug 11, 2017

mikaelarguedas commented Aug 11, 2017

allenh1 commented Aug 11, 2017

allenh1 commented Aug 13, 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

mikaelarguedas commented Aug 23, 2017

mikaelarguedas commented Aug 30, 2017

allenh1 commented Aug 31, 2017

allenh1 commented Aug 31, 2017 • edited Loading

mikaelarguedas commented Aug 31, 2017

allenh1 commented Sep 1, 2017

mikaelarguedas commented Sep 1, 2017

allenh1 commented Aug 13, 2017 •

edited

Loading

allenh1 commented Aug 31, 2017 •

edited

Loading