Skip to content

Commit

Permalink
Better support and testing for empty string_array (#246)
Browse files Browse the repository at this point in the history
Also don't fail due to implementation-specific malloc behavior.

The documentation for malloc states that an attempt to allocate memory
of size 0 results in implementation-specific behavior. Some
implementations return `NULL`, and others return a pointer that is
expected to be freed.

Signed-off-by: Scott K Logan <logans@cottsay.net>
  • Loading branch information
cottsay authored May 27, 2020
1 parent 78f2d14 commit f9bf1f0
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 5 deletions.
13 changes: 8 additions & 5 deletions src/string_array.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ rcutils_string_array_init(
}
string_array->size = size;
string_array->data = allocator->zero_allocate(size, sizeof(char *), allocator->state);
if (NULL == string_array->data) {
if (NULL == string_array->data && 0 != size) {
RCUTILS_SET_ERROR_MSG("failed to allocator string array");
return RCUTILS_RET_BAD_ALLOC;
}
Expand Down Expand Up @@ -98,10 +98,6 @@ rcutils_string_array_cmp(
lhs, "lhs string array is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
rhs, "rhs string array is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
lhs->data, "lhs->data is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
rhs->data, "rhs->data is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
res, "res argument is null", return RCUTILS_RET_INVALID_ARGUMENT);

Expand All @@ -110,6 +106,13 @@ rcutils_string_array_cmp(
smallest_size = rhs->size;
}

if (smallest_size > 0) {
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
lhs->data, "lhs->data is null", return RCUTILS_RET_INVALID_ARGUMENT);
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
rhs->data, "rhs->data is null", return RCUTILS_RET_INVALID_ARGUMENT);
}

for (size_t i = 0; i < smallest_size; ++i) {
RCUTILS_CHECK_FOR_NULL_WITH_MSG(
lhs->data[i], "lhs array element is null", return RCUTILS_RET_ERROR);
Expand Down
10 changes: 10 additions & 0 deletions test/test_string_array.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ TEST(test_string_array, boot_string_array) {
rcutils_reset_error();
sa3.allocator = allocator;
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_fini(&sa3));

rcutils_string_array_t sa4 = rcutils_get_zero_initialized_string_array();
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_init(&sa4, 0, &allocator));
ASSERT_EQ(0u, sa4.size);
}

TEST(test_string_array, string_array_cmp) {
Expand Down Expand Up @@ -96,6 +100,7 @@ TEST(test_string_array, string_array_cmp) {
sa3.data[0] = strdup("foo");
sa3.data[1] = strdup("bar");

rcutils_string_array_t empty_string_array = rcutils_get_zero_initialized_string_array();
rcutils_string_array_t incomplete_string_array = rcutils_get_zero_initialized_string_array();
ret = rcutils_string_array_init(&incomplete_string_array, 3, &allocator);
ASSERT_EQ(RCUTILS_RET_OK, ret);
Expand Down Expand Up @@ -126,6 +131,11 @@ TEST(test_string_array, string_array_cmp) {
// Test transitivity
EXPECT_EQ(RCUTILS_RET_OK, rcutils_string_array_cmp(&sa3, &sa2, &res));
EXPECT_LT(res, 0);
// Test empty
EXPECT_EQ(RCUTILS_RET_OK, rcutils_string_array_cmp(&sa0, &empty_string_array, &res));
EXPECT_GT(res, 0);
EXPECT_EQ(RCUTILS_RET_OK, rcutils_string_array_cmp(&empty_string_array, &sa0, &res));
EXPECT_LT(res, 0);

ret = rcutils_string_array_fini(&sa0);
ASSERT_EQ(RCUTILS_RET_OK, ret);
Expand Down

0 comments on commit f9bf1f0

Please sign in to comment.