[rcl_action] Implement init/fini functions for types by jacobperron · Pull Request #312 · ros2/rcl

jacobperron · 2018-10-30T00:41:38Z

Connects to #306.

Refactored API. Removed init/fini functions for types that do not necessarily need allocation on the heap.
Added unit tests for implementation.

jacobperron · 2018-10-30T01:11:27Z

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-10-30T13:19:14Z

rcl_action/src/rcl_action/types.c

+rcl_action_goal_info_t
+rcl_action_get_zero_initialized_goal_info(void)
+{
+  static rcl_action_goal_info_t goal_info = {


@jacobperron I wonder if just goal_info = {0} would do for all compilers (as it does for gcc).

Yes, that will work. For some reason I thought the internal uuid array wasn't initializing with the proper size.

hidmic · 2018-10-30T13:22:24Z

rcl_action/src/rcl_action/types.c

+rcl_action_get_zero_initialized_cancel_request(void)
+{
+  static rcl_action_cancel_request_t request;
+  request.goal_info = rcl_action_get_zero_initialized_goal_info();


@jacobperron this one's tricky, we rely on the compiler to be smart enough to not call rcl_action_get_zero_initialized_goal_info() every single time. How wrong would it be to go with request = {0}; only?

hidmic · 2018-10-30T13:22:36Z

rcl_action/src/rcl_action/types.c

+rcl_action_cancel_response_t
+rcl_action_get_zero_initialized_cancel_response(void)
+{
+  static rcl_action_cancel_response_t response;


@jacobperron missing zero initialization?

hidmic · 2018-10-30T13:24:21Z

rcl_action/test/rcl_action/test_types.cpp

+TEST(TestActionTypes, test_get_zero_inititalized_goal_info)
+{
+  rcl_action_goal_info_t goal_info = rcl_action_get_zero_initialized_goal_info();
+  ASSERT_EQ(sizeof(goal_info.uuid) / sizeof(uint8_t), 16lu);


@jacobperron that long literal qualifier is painfully similar to a 1, I wonder which compiler complains if missing.

Changing to 16u seems okay.

hidmic · 2018-10-30T13:26:24Z

rcl_action/test/rcl_action/test_types.cpp

+  goal_info.stamp.nanosec = 4567u;
+  rcl_action_goal_info_t another_goal_info = rcl_action_get_zero_initialized_goal_info();
+  for (int i = 0; i < 16; ++i) {
+    EXPECT_EQ(goal_info.uuid[i], i);


@jacobperron what's the rationale for this test? A comment explaining it would be super helpful.

This was more of a sanity check for myself that creating a second object with the function did not interfere with the first. I'm okay to remove the second goal test if you think it's too pedantic.

I think it's a good test, but I would add a comment explaining it for the unaware reader.

hidmic · 2018-10-30T13:27:24Z

rcl_action/test/rcl_action/test_types.cpp

+TEST(TestActionTypes, test_get_zero_initialized_cancel_response)
+{
+  rcl_action_cancel_response_t cancel_response = rcl_action_get_zero_initialized_cancel_response();
+  ASSERT_EQ(cancel_response.goals_canceling.size, 0lu);


@jacobperron shouldn't we check for the pointer to be NULL too?

Added check.

hidmic · 2018-10-30T13:28:36Z

rcl_action/test/rcl_action/test_types.cpp

+  // Initialize with invalid status array
+  rcl_ret_t ret = rcl_action_goal_status_array_init(
+    nullptr,
+    (size_t) 3,


@jacobperron consider making these two above constants for better readability and deduplication.

hidmic

Two extra tiny bits.

hidmic · 2018-10-30T19:38:15Z

rcl_action/src/rcl_action/types.c

+rcl_action_goal_status_array_t
+rcl_action_get_zero_initialized_goal_status_array(void)
+{
+  static rcl_action_goal_status_array_t status_array;


@jacobperron missing zero initialization?

hidmic · 2018-10-30T19:45:29Z

rcl_action/test/rcl_action/test_types.cpp

+{
+  rcl_action_goal_status_array_t status_array =
+    rcl_action_get_zero_initialized_goal_status_array();
+  ASSERT_EQ(sizeof(status_array.status_list) / sizeof(rcl_action_goal_status_t), 0u);


@jacobperron what's this intent of this test? Won't sizeof(status_array.status_list) be always the same?

hidmic · 2018-10-30T19:46:55Z

Also, it looks like MSVC is complaining about linkage again -.-

jacobperron · 2018-10-30T20:04:54Z

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2018-10-30T20:05:39Z

Also, it looks like MSVC is complaining about linkage again -.-

Forgot to uncomment the cmake line that should resolve this

jacobperron · 2018-10-30T20:16:07Z

Also, it looks like Clang wants braces around initialization of sub-objects in the zero init functions.

jacobperron · 2018-10-30T20:29:43Z

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-10-30T21:30:43Z

Looks like Clang is a bit picky.

jacobperron · 2018-10-30T22:48:26Z

Up to rcl_action:

Linux
Linux-aarch64
macOS
Windows

🎉

Refactored API. Removed init/fini functions for types that do not necessarily need allocation on the heap. Added unit tests for implementation.

- Simplify and add missing zero initializers - Replace `lu` qualifier with `u` - Add null ptr check in cancel response zero init test - Make size arguments constants

This is to satisfy Clang warnings.

jacobperron · 2018-10-30T23:40:55Z

Rebased on master (resolving conflicts in CMakeLists.txt).

CI (up to rcl_action):

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM!

hidmic · 2018-10-31T20:14:31Z

rcl_action/test/rcl_action/test_types.cpp

+{
+  rcl_action_goal_status_array_t status_array =
+    rcl_action_get_zero_initialized_goal_status_array();
+  // ASSERT_EQ(sizeof(status_array.status_list.data) / sizeof(rcl_action_goal_status_t), 0u);


@jacobperron nit: remove the comment.

sloretz

I'm a bit late with the review, just some minor comments

sloretz · 2018-10-31T20:46:36Z

rcl_action/CMakeLists.txt

  ${rcl_action_sources}
 )
+target_link_libraries(${PROJECT_NAME}
+  ${rcl_LIBRARIES}


I think ament_target_dependencies("rcl"...) below does this already

https://github.com/ament/ament_cmake/blob/88add8cab0ca579e89e95b3ab343af319c163195/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake#L55

sloretz · 2018-10-31T20:49:46Z

rcl_action/include/rcl_action/types.h

-rcl_action_get_zero_initialized_cancel_response(void);
+rcl_action_goal_status_array_fini(
+  rcl_action_goal_status_array_t * status_array,
+  const rcl_allocator_t allocator);


Recommend storing allocator in rcl_action_goal_status_array_t to avoid accidentally deallocating with the wrong allocator.

Currently, it is defined as:

typedef action_msgs__msg__GoalStatusArray rcl_action_goal_status_array_t;

If we want to include space for the allocator, then I suppose we should do something like:

typedef struct rcl_action_goal_status_array_t { action_msgs__msg__GoalStatusArray msg; rcl_allocator_t allocator; } rcl_action_goal_status_array_t;

I can make a follow-up PR to address this and your other comments.

👍 came here to say this, I noticed it in the API when rebasing my pr (#314).

sloretz · 2018-10-31T20:50:39Z

rcl_action/include/rcl_action/types.h

 rcl_action_cancel_response_fini(
  rcl_action_cancel_response_t * cancel_response,
-  rcl_action_server_t * action_server);
+  const rcl_allocator_t allocator);


Same comment about storing allocator rather than passing it to fini

sloretz · 2018-10-31T20:52:40Z

rcl_action/src/rcl_action/types.c

+  RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(status_array, RCL_RET_INVALID_ARGUMENT, allocator);
+  // Ensure status array is zero initialized
+  if (status_array->status_list.size > 0) {


What should this function do if it is called with num_status = 0?

sloretz · 2018-10-31T20:54:47Z

rcl_action/src/rcl_action/types.c

+  RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(cancel_response, RCL_RET_INVALID_ARGUMENT, allocator);
+  // Ensure cancel response is zero initialized
+  if (cancel_response->goals_canceling.size > 0) {


What should this function do if num_goals_canceling = 0?

The default allocator uses calloc, which has undefined behavior if the size is zero

If size is zero, the return value depends on the particular library implementation (it may or may not be a null pointer), but the returned pointer shall not be dereferenced.

http://www.cplusplus.com/reference/cstdlib/calloc/

Good catch. Perhaps it should return RCL_RET_INVALID_ARGUMENT?

jacobperron 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 Oct 30, 2018

jacobperron mentioned this pull request Oct 30, 2018

Actions #306

Closed

10 tasks

hidmic reviewed Oct 30, 2018

View reviewed changes

jacobperron force-pushed the jacob/action_types branch from af424ec to 0a90b35 Compare October 30, 2018 18:28

hidmic reviewed Oct 30, 2018

View reviewed changes

jacobperron closed this Oct 30, 2018

jacobperron reopened this Oct 30, 2018

jacobperron added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 30, 2018

jacobperron self-assigned this Oct 30, 2018

jacobperron added 8 commits October 30, 2018 16:32

[rcl_action] Implement init/fini functions for types

00f70ab

Refactored API. Removed init/fini functions for types that do not necessarily need allocation on the heap. Added unit tests for implementation.

Address reviewers comments

47a6ae1

- Simplify and add missing zero initializers - Replace `lu` qualifier with `u` - Add null ptr check in cancel response zero init test - Make size arguments constants

Add missing zero initialization

3f75211

Update zero init test

bddaed6

Add compile defintion for rcl_action dll

3bfac6c

Add braces around sub-objects in zero initializes

3e0ccb9

This is to satisfy Clang warnings.

more braces

03c8b92

Clarify test

7ee1e56

jacobperron force-pushed the jacob/action_types branch from ea0d79c to 7ee1e56 Compare October 30, 2018 23:39

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 31, 2018

hidmic approved these changes Oct 31, 2018

View reviewed changes

Remove leftover comment

8e2f1a0

jacobperron merged commit f9dfc5d into master Oct 31, 2018

jacobperron deleted the jacob/action_types branch October 31, 2018 20:36

jacobperron removed the in review Waiting for review (Kanban column) label Oct 31, 2018

sloretz reviewed Oct 31, 2018

View reviewed changes

Conversation

jacobperron commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

hidmic commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

jacobperron commented Oct 30, 2018

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!