add rcutils_unsigned_char_array_t #125

Karsten1987 · 2018-11-06T23:59:10Z

This data structure will eventually be used for serialized messages and replaces the rcutils_char_array_t for binary data buffers.
initial issue here: ros2/demos#279

CI:

wjwwood · 2018-11-07T00:09:31Z

I'm not up to speed on this issue, but what about using uint8_t rather than unsigned char? There might be a reason not to do so, but it seems like the least ambiguous type to me for serialized data.

Karsten1987 · 2018-11-07T00:13:11Z

I am open to new types and names here. I thought unsigned char is just a typedef to uint8_t and thus chose rcutils_unsigned_char_array_t to make the contrast to rcutils_signed_char_array_t clear.

Is there any reason to prefer uin8_t over unsigned char ?

clalancette · 2018-11-07T13:47:25Z

Is there any reason to prefer uin8_t over unsigned char ?

There is one (pretty pedantic) reason that @sloretz brought up; char is not guaranteed to be 8-bits by the standard. All implementations that we care about make it 8 bits, of course, but naming it uint8_t just removes all possible ambiguity. So I'd agree with @wjwwood on naming it uint8_array_t.

wjwwood · 2018-11-07T16:48:47Z

char is not guaranteed to be 8-bits by the standard.

The question then becomes, if char is not 8-bit, then should our serialized data be an array of 8-bit values or what ever the size of char is? 😁

It's a bit academic, because I don't think it will "just" work on a machine where char is not 8-bit, and I think we're not likely to find such a machine any time soon. But I do think uint8_t is just less ambiguous.

allenh1 · 2018-11-07T17:32:16Z

Is there any reason not to call this rcutils_byte_array_t?

wjwwood · 2018-11-07T18:15:52Z

rcutils_byte_array_t would also be fine as it is a commonly used term (octet is another example of a term that isn't a type), but byte isn't a C/C++ type so I'd give slight preference to using an explicit type name.

Not that it would require a change in the name of this struct, but MISRA requires that you use explicitly sized types, like int32_t or int64_t rather than int, and under the same idea or guidance, using an explicitly sized type in the name here makes sense to me.

allenh1 · 2018-11-07T18:48:05Z

@wjwwood that makes sense -- and +1 for the uint8_t idea.

Karsten1987 · 2018-11-08T01:34:28Z

renamed to rcutils_uint8_array

CI:

Karsten1987 · 2018-11-12T19:45:42Z

@ros2/team friendly ping :)

include/rcutils/types/uint8_array.h

wjwwood · 2018-11-12T19:53:06Z

include/rcutils/types/uint8_array.h

+ * Be aware, that this will deallocate the memory and therefore invalidates any
+ * pointers to this storage.
+ * If the new size is larger, new memory is getting allocated and the existing
+ * content is copied over.


Are you using realloc internally? And if so do you also use it when shrinking the data? And if so does that not also potentially mean the pointer address can change?

I see now that you are, so this applies I believe:

the original pointer ptr is invalidated and any access to it is undefined behavior (even if reallocation was in-place).
-- https://en.cppreference.com/w/cpp/memory/c/realloc

yes, the pointer might change. Is there any way around that?

Is there any way around that?

In which case? In general, no I don't think so.

We have it documented just before this sentence that all pointers to the underlying buffer will be invalidated. What do you advise to do here?

It says that will only happen "if the new size is larger", which is not the only case and the reason I brought it up initially.

At least it should be stated that you should always assume the pointer is invalid after calling this, regardless of the input arguments and how they related to current values.

include/rcutils/types/uint8_array.h

src/uint8_array.c

jacobperron · 2018-11-12T21:21:54Z

src/uint8_array.c

+    "uint8 array pointer is null",
+    return RCUTILS_RET_ERROR);
+
+  if (!rcutils_allocator_is_valid(allocator)) {


RCUTILS_CHECK_ALLOCATOR(allocator, return RCUTILS_RET_INVALID_ARGUMENT);

Same for other occurrences below.

jacobperron · 2018-11-12T21:23:16Z

src/uint8_array.c

+rcutils_ret_t
+rcutils_uint8_array_fini(rcutils_uint8_array_t * uint8_array)
+{
+  RCUTILS_CHECK_FOR_NULL_WITH_MSG(


RCUTILS_CHECK_ARGUMENT_FOR_NULL(uint8_array, RCUTILS_RET_INVALID_ARGUMENT);

Same for other occurrences below.

Karsten1987 · 2018-11-12T21:31:04Z

thanks for the feedback. Given that this uint8_array is a direct copy of the char_array I believe the same comments apply to these files as well.
Any opinion whether I should fix them in this PR or have a follow up PR on the char array?

wjwwood · 2018-11-12T21:40:01Z

Any opinion whether I should fix them in this PR or have a follow up PR on the char array?

I think it doesn't make sense to merge this with known issues and fix it later, so I'd go with fix in place, retest and merge, then follow up on char array.

Whether or not you follow up on char array in this pr or in a new one doesn't matter to me.

Karsten1987 · 2018-11-12T22:55:54Z

Hopefully addressed all comments. I decided to iterate over the changes here at first and then apply the same to rcutils_char_array in a new PR once this PR is in.

include/rcutils/types/uint8_array.h

wjwwood · 2018-11-12T23:10:08Z

include/rcutils/types/uint8_array.h

+ * Be aware, that this will deallocate the memory and therefore invalidates any
+ * pointers to this storage.
+ * If the new size is larger, new memory is getting allocated and the existing
+ * content is copied over.


It says that will only happen "if the new size is larger", which is not the only case and the reason I brought it up initially.

At least it should be stated that you should always assume the pointer is invalid after calling this, regardless of the input arguments and how they related to current values.

Co-Authored-By: Karsten1987 <[email protected]>

Karsten1987 · 2018-11-21T01:36:52Z

@wjwwood do you mind checking it again? I would like to have this in soon so I can build on top of that for the serialized messages.

wjwwood

Lgtm, sorry if I was holding it up.

Karsten1987 · 2018-11-21T02:31:00Z

thanks for the review!

add rcutils_unsigned_char_array_t

84e7201

Karsten1987 added the in review Waiting for review (Kanban column) label Nov 6, 2018

Karsten1987 self-assigned this Nov 6, 2018

Karsten1987 added 2 commits November 7, 2018 13:17

unsigned char to uint8

9601be5

include stdint.h

d3e6938

Karsten1987 force-pushed the rcutils_unsigned_char_array_t branch from 4f63b7e to d3e6938 Compare November 7, 2018 23:39

wjwwood reviewed Nov 12, 2018

View reviewed changes

jacobperron reviewed Nov 12, 2018

View reviewed changes

address comments

56e48d2

wjwwood reviewed Nov 12, 2018

View reviewed changes

wjwwood and others added 4 commits November 12, 2018 15:21

Update include/rcutils/types/uint8_array.h

d40ed61

Co-Authored-By: Karsten1987 <[email protected]>

Update include/rcutils/types/uint8_array.h

6a2844d

Co-Authored-By: Karsten1987 <[email protected]>

Update include/rcutils/types/uint8_array.h

a3842e4

Co-Authored-By: Karsten1987 <[email protected]>

allow RCUTILS_RET_ERROR

06702b0

wjwwood approved these changes Nov 21, 2018

View reviewed changes

Karsten1987 merged commit f715e16 into master Nov 21, 2018

Karsten1987 deleted the rcutils_unsigned_char_array_t branch November 21, 2018 02:30

Karsten1987 removed the in review Waiting for review (Kanban column) label Nov 21, 2018

Karsten1987 mentioned this pull request Nov 25, 2018

reiterate over char array #130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rcutils_unsigned_char_array_t #125

add rcutils_unsigned_char_array_t #125

Karsten1987 commented Nov 6, 2018

wjwwood commented Nov 7, 2018

Karsten1987 commented Nov 7, 2018

clalancette commented Nov 7, 2018

wjwwood commented Nov 7, 2018

allenh1 commented Nov 7, 2018

wjwwood commented Nov 7, 2018 •

edited

Loading

allenh1 commented Nov 7, 2018

Karsten1987 commented Nov 8, 2018

Karsten1987 commented Nov 12, 2018

wjwwood Nov 12, 2018

wjwwood Nov 12, 2018

Karsten1987 Nov 12, 2018

wjwwood Nov 12, 2018

Karsten1987 Nov 12, 2018

wjwwood Nov 12, 2018

jacobperron Nov 12, 2018

jacobperron Nov 12, 2018

Karsten1987 commented Nov 12, 2018

wjwwood commented Nov 12, 2018

Karsten1987 commented Nov 12, 2018

wjwwood Nov 12, 2018

Karsten1987 commented Nov 21, 2018

wjwwood left a comment

Karsten1987 commented Nov 21, 2018

add rcutils_unsigned_char_array_t #125

add rcutils_unsigned_char_array_t #125

Conversation

Karsten1987 commented Nov 6, 2018

wjwwood commented Nov 7, 2018

Karsten1987 commented Nov 7, 2018

clalancette commented Nov 7, 2018

wjwwood commented Nov 7, 2018

allenh1 commented Nov 7, 2018

wjwwood commented Nov 7, 2018 • edited Loading

allenh1 commented Nov 7, 2018

Karsten1987 commented Nov 8, 2018

Karsten1987 commented Nov 12, 2018

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

Karsten1987 commented Nov 12, 2018

wjwwood commented Nov 12, 2018

Karsten1987 commented Nov 12, 2018

Choose a reason for hiding this comment

Karsten1987 commented Nov 21, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Karsten1987 commented Nov 21, 2018

wjwwood commented Nov 7, 2018 •

edited

Loading