Adding an ArrayList and HashMap implementation to rcutils #131

nburek · 2018-12-05T03:32:39Z

This change adds an ArrayList and HashMap implementation to rcutils. These data structures were needed for the rosout logging feature so that we could easily map logger names to nodes/publishers.

These changes were made on top of commits being reviewed in pull request #127. Once that pull request is merged on the latest commit that is on top of that will show in this PR.

hidmic

@nburek Awesome contribution! And sorry for the review comments avalanche, it's a big PR.

hidmic · 2019-01-04T14:03:24Z

include/rcutils/types/array_list.h

@@ -0,0 +1,208 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.


@nburek consider documenting thread safety, memory allocation, atomic usage and lock usage for each function in this API (as it's done here).

Added this documentation.

hidmic · 2019-01-04T14:16:03Z

include/rcutils/types/array_list.h

+rcutils_array_list_t
+rcutils_get_zero_initialized_array_list(void);
+
+/// Initialize an array liast with a given initial capacity.


@nburek liast -> list

hidmic · 2019-01-04T14:16:36Z

include/rcutils/types/array_list.h

+  size_t data_size,
+  const rcutils_allocator_t * allocator);
+
+/// Finalize a string array, reclaiming all resources.


@nburek string array -> array list? It looks like there're a bunch of refactor leftovers here and there in this file :).

This data structure started as a more generic copy of the string array and then evolved to its current state (same with the hash_map). Sorry for the leftover cruft, I thought I had already scrubbed the documentation before I pushed but I guess I forgot.

No worries.

hidmic · 2019-01-04T14:20:04Z

include/rcutils/types/array_list.h

+ * array_list.data[0] = rcutils_strdup("Hello", &allocator);
+ * array_list.data[1] = rcutils_strdup("World", &allocator);
+ * ret = rcutils_array_list_fini(&array_list);
+ *


@nburek missing closing code block backticks?

Yep, they got pushed bellow some of the other comments that weren't meant to be code. Fixed.

hidmic · 2019-01-04T14:20:50Z

include/rcutils/types/array_list.h

+ *   // ... error handling
+ * }
+ * array_list.data[0] = rcutils_strdup("Hello", &allocator);
+ * array_list.data[1] = rcutils_strdup("World", &allocator);


@nburek it looks like this example is out of date.

Fixed this example and a number of others.

hidmic · 2019-01-04T15:26:15Z

src/hash_map.c

+  HASH_MAP_VALIDATE_ARGUMENT_NOT_NULL(value);
+
+  size_t key_hash = 0, map_index = 0, bucket_index = 0;
+  bool already_exists = false;


@nburek nit: why not declaring it at the time of first use?

It's just grouping declarations near the top. They're all used shortly after.

hidmic · 2019-01-04T15:26:38Z

src/hash_map.c

+  HASH_MAP_VALIDATE_ARGUMENT_NOT_NULL(key);
+
+  size_t key_hash = 0, map_index = 0, bucket_index = 0;
+  bool already_exists = false;


@nburek nit: why not declaring it at the time of first use?

Same as above.

hidmic · 2019-01-04T15:30:10Z

src/hash_map.c

+  rcutils_ret_t ret = RCUTILS_RET_OK;
+
+  if (NULL != previous_key) {
+    already_exists = hash_map_find(hash_map, key, &key_hash, &map_index, &bucket_index, &entry);


@nburek meta^2: iteration looks expensive, I wonder if there's a cheaper way to track where did it left off the last time rcutils_hash_map_get_next_key_and_data was called.

In the case where you're not modifying the map as you iterate through then this shouldn't be that much more expensive. The find should average out to be constant time. There would be some slight benefit in creating an iterator style method for this as it might not need to rehash the key each time. I didn't use the iterator approach for two reasons:

My primary use case was to use this function when cleaning up the map and I would be modifying it by removing entries as I went. A simple iterator would be invalid if the map was modified in between.

So far, the other data structures didn't look like they were using an iterator style approach. This function was more in line with what I was seeing from the other data structures. I didn't want to risk add complexity to this review by trying to set a precedent for iterators when it wasn't needed yet.

I see. And I agree with you. Thanks for the detailed reply.

hidmic · 2019-01-04T15:33:22Z

test/test_array_list.cpp

@@ -0,0 +1,355 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.


@nburek 2017 -> 2019?

hidmic · 2019-01-04T15:37:48Z

test/test_hash_map.cpp

@@ -0,0 +1,452 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.


@nburek 2017 -> 2019?

nburek · 2019-01-07T18:40:30Z

@hidmic Thank you for the thorough review of the documentation. There were a lot of little fixups in there that got past me.

I've posted an updated version based on your comments.

hidmic

LGTM but for a few minor nits and a question. Also pending green CI.

include/rcutils/types/hash_map.h

hidmic · 2019-01-08T14:15:12Z

include/rcutils/types/hash_map.h

+ * The order of the keys in the hash_map is arbitrary and if the hash_map is modified
+ * between calls to this function the behavior is undefined.
+ * If the hash_map is modifeid then iteration should begin again by passing NULL to
+ * get the first key again.


@nburek hmm, isn't this statement against your use case in rcl?

Yes, I'll correct the rcl review.

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

nburek · 2019-01-08T17:05:13Z

Fixed the additional typos you pointed out.

It looks like the build is failing because the uncrustify linter is detecting that the logging_macros.h file is out of compliance. This file is auto generated from logging_macros.h.em and it looks like the failure may be a result of this PR: #133

Since the failure appears to be unrelated to my PR I would prefer not to fix it here.

hidmic · 2019-01-08T17:22:13Z

Not sure how could that be the case. IIRC #133 should make uncrustify ignore line lengths. @jacobperron will probably know better though.

In any case, this still needs to go through https://ci.ros2.org. I'll set it up for you.

hidmic · 2019-01-08T18:06:03Z

Running CI, including this PR and ros2/rcl#350:

Linux
Linux-aarch64
macOS
Windows

chapulina · 2019-01-10T00:05:15Z

Looking at CI, it looks like this PR by itself only has a few uncrustify_logging_macros errors (see Cpr...), but the combination of this PR with ros2/rcl#350 is causing a lot of test failures. I think it would be good to figure out what's causing those errors before merging this PR, just in case.

nuclearsandwich · 2019-01-10T00:06:35Z

this PR by itself only has a few uncrustify_logging_macros errors (see Cpr...)

Those are due to the upgrade to uncrustify 0.68.1 on master while we released Crystal with 0.67 and they disagree on a few points. We're upgrading Crystal to 0.68.1 in the upcoming patch release and these errors should subside.

nburek · 2019-01-11T00:34:32Z

Commented on the other PR as well, but the bug in the changes to rcl that were causing the failures due to double free is now fixed. Can you please run the CI pipeline again when you get a chance? Thank you.

nburek · 2019-01-11T00:46:02Z

I take that back, there is one more broken test in rcl related to test_graph because there is now always one extra topic running on the node for rosout. Fixing that now.

nburek · 2019-01-11T00:47:52Z

Okay, now it should be ready for retest.

chapulina · 2019-01-11T01:06:14Z

Here you go:

Linux
Linux-aarch64
macOS
Windows

jacobperron

Just some very minor comments. LGTM

include/rcutils/types/array_list.h

src/hash_map.c

nburek · 2019-01-16T22:06:16Z

Can we try to get this PR merged into the next Crystal patch release?
ros2/ros2#647

nuclearsandwich · 2019-01-18T01:17:07Z

Can we try to get this PR merged into the next Crystal patch release?

This is queued for patch release 2.

tfoote added the in review Waiting for review (Kanban column) label Dec 5, 2018

nburek force-pushed the rosout branch 2 times, most recently from 895cb6a to e3347c9 Compare December 7, 2018 06:05

hidmic requested changes Jan 4, 2019

View reviewed changes

nburek added 2 commits January 4, 2019 08:54

Adding an ArrayList and HashMap data structures along with tests.

6eec102

Fixup for PR.

e981a92

nburek force-pushed the rosout branch from e3347c9 to e981a92 Compare January 7, 2019 18:39

clalancette mentioned this pull request Jan 7, 2019

ROS 2 Crystal Clemmys Patch Release 1 ros2/ros2#638

Closed

hidmic approved these changes Jan 8, 2019

View reviewed changes

hidmic and others added 3 commits January 8, 2019 08:42

Fix typo.

1453fe5

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

Fix typo.

ba6ebdd

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

Fixing typo.

55148b7

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

hidmic mentioned this pull request Jan 8, 2019

Publish logs to Rosout ros2/rcl#350

Merged

jacobperron approved these changes Jan 14, 2019

View reviewed changes

include/rcutils/types/array_list.h Show resolved Hide resolved

include/rcutils/types/array_list.h Outdated Show resolved Hide resolved

src/hash_map.c Outdated Show resolved Hide resolved

src/hash_map.c Outdated Show resolved Hide resolved

Minor fixup for PR.

2098437

jacobperron merged commit b98ac15 into ros2:master Jan 14, 2019

jacobperron removed the in review Waiting for review (Kanban column) label Jan 14, 2019

nuclearsandwich mentioned this pull request Jan 18, 2019

Crystal Patch Release 2 ros2/ros2#647

Closed

This was referenced Feb 8, 2019

Backport #384 for Crystal. ros2/rcl#385

Merged

Backport #368 for Crystal ros2/rcl#386

Merged

		@@ -0,0 +1,208 @@
		// Copyright 2017 Open Source Robotics Foundation, Inc.

		@@ -0,0 +1,355 @@
		// Copyright 2017 Open Source Robotics Foundation, Inc.

		@@ -0,0 +1,452 @@
		// Copyright 2017 Open Source Robotics Foundation, Inc.

Adding an ArrayList and HashMap implementation to rcutils #131

Adding an ArrayList and HashMap implementation to rcutils #131

Conversation

nburek commented Dec 5, 2018

hidmic 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nburek commented Jan 7, 2019

hidmic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nburek commented Jan 8, 2019

hidmic commented Jan 8, 2019

hidmic commented Jan 8, 2019

chapulina commented Jan 10, 2019

nuclearsandwich commented Jan 10, 2019

nburek commented Jan 11, 2019

nburek commented Jan 11, 2019

nburek commented Jan 11, 2019

chapulina commented Jan 11, 2019

jacobperron left a comment

Choose a reason for hiding this comment

nburek commented Jan 16, 2019

nuclearsandwich commented Jan 18, 2019

hidmic left a comment •

edited

Loading