From 9f7f8b23987de99ba5b4d87dfba9f64f00f1f9e9 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Thu, 5 Nov 2020 15:03:51 +0800 Subject: [PATCH 1/3] refactor for removing unnecessary source code Signed-off-by: Chen Lihui --- rcl/src/rcl/init_options.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/rcl/src/rcl/init_options.c b/rcl/src/rcl/init_options.c index 9f88b2166..303139c7a 100644 --- a/rcl/src/rcl/init_options.c +++ b/rcl/src/rcl/init_options.c @@ -87,26 +87,7 @@ rcl_init_options_copy(const rcl_init_options_t * src, rcl_init_options_t * dst) } // copy src information into dst - dst->impl->allocator = src->impl->allocator; - // first zero-initialize rmw init options - rmw_ret_t rmw_ret = rmw_init_options_fini(&(dst->impl->rmw_init_options)); - if (RMW_RET_OK != rmw_ret) { - rmw_error_string_t error_string = rmw_get_error_string(); - rmw_reset_error(); - ret = rcl_init_options_fini(dst); - if (RCL_RET_OK != ret) { - RCUTILS_LOG_ERROR_NAMED( - "rcl", - "failed to finalize dst rcl_init_options while handling failure to " - "finalize rmw_init_options, original ret '%d' and error: %s", rmw_ret, error_string.str); - return ret; // error already set - } - RCL_SET_ERROR_MSG(error_string.str); - return rcl_convert_rmw_ret_to_rcl_ret(rmw_ret); - } - // then copy - dst->impl->rmw_init_options = rmw_get_zero_initialized_init_options(); - rmw_ret = + rmw_ret_t rmw_ret = rmw_init_options_copy(&(src->impl->rmw_init_options), &(dst->impl->rmw_init_options)); if (RMW_RET_OK != rmw_ret) { rmw_error_string_t error_string = rmw_get_error_string(); From 4fcec36699f4d166ea7cfe7b3d902d346729afd6 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Thu, 5 Nov 2020 18:03:01 +0800 Subject: [PATCH 2/3] Add a helper function that make rcl_init_options_copy not to call rmw_init_options_fini remove unnecessary test Signed-off-by: Chen Lihui --- rcl/src/rcl/init_options.c | 31 +++++++++++++++++++++++-------- rcl/test/rcl/test_init.cpp | 17 ----------------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/rcl/src/rcl/init_options.c b/rcl/src/rcl/init_options.c index 303139c7a..965d72178 100644 --- a/rcl/src/rcl/init_options.c +++ b/rcl/src/rcl/init_options.c @@ -34,6 +34,22 @@ rcl_get_zero_initialized_init_options(void) }; // NOLINT(readability/braces): false positive } +/// Initialize given init_options with the default values and zero-initialize implementation. +RCL_LOCAL +rcl_ret_t +_rcl_init_options_init(rcl_init_options_t * init_options, rcl_allocator_t allocator) +{ + init_options->impl = allocator.allocate(sizeof(rcl_init_options_impl_t), allocator.state); + RCL_CHECK_FOR_NULL_WITH_MSG( + init_options->impl, + "failed to allocate memory for init options impl", + return RCL_RET_BAD_ALLOC); + init_options->impl->allocator = allocator; + init_options->impl->rmw_init_options = rmw_get_zero_initialized_init_options(); + + return RCL_RET_OK; +} + rcl_ret_t rcl_init_options_init(rcl_init_options_t * init_options, rcl_allocator_t allocator) { @@ -48,13 +64,11 @@ rcl_init_options_init(rcl_init_options_t * init_options, rcl_allocator_t allocat return RCL_RET_ALREADY_INIT; } RCL_CHECK_ALLOCATOR(&allocator, return RCL_RET_INVALID_ARGUMENT); - init_options->impl = allocator.allocate(sizeof(rcl_init_options_impl_t), allocator.state); - RCL_CHECK_FOR_NULL_WITH_MSG( - init_options->impl, - "failed to allocate memory for init options impl", - return RCL_RET_BAD_ALLOC); - init_options->impl->allocator = allocator; - init_options->impl->rmw_init_options = rmw_get_zero_initialized_init_options(); + + rcl_ret_t ret = _rcl_init_options_init(init_options, allocator); + if (RCL_RET_OK != ret) { + return ret; + } rmw_ret_t rmw_ret = rmw_init_options_init(&(init_options->impl->rmw_init_options), allocator); if (RMW_RET_OK != rmw_ret) { allocator.deallocate(init_options->impl, allocator.state); @@ -74,6 +88,7 @@ rcl_init_options_copy(const rcl_init_options_t * src, rcl_init_options_t * dst) RCL_CHECK_ARGUMENT_FOR_NULL(src, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(src->impl, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ALLOCATOR(&src->impl->allocator, return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(dst, RCL_RET_INVALID_ARGUMENT); if (NULL != dst->impl) { RCL_SET_ERROR_MSG("given dst (rcl_init_options_t) is already initialized"); @@ -81,7 +96,7 @@ rcl_init_options_copy(const rcl_init_options_t * src, rcl_init_options_t * dst) } // initialize dst (since we know it's in a zero initialized state) - rcl_ret_t ret = rcl_init_options_init(dst, src->impl->allocator); + rcl_ret_t ret = _rcl_init_options_init(dst, src->impl->allocator); if (RCL_RET_OK != ret) { return ret; // error already set } diff --git a/rcl/test/rcl/test_init.cpp b/rcl/test/rcl/test_init.cpp index dd3c119c0..72913c258 100644 --- a/rcl/test/rcl/test_init.cpp +++ b/rcl/test/rcl/test_init.cpp @@ -548,23 +548,6 @@ TEST_F(CLASSNAME(TestRCLFixture, RMW_IMPLEMENTATION), test_mocked_rcl_init_optio EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options)); } -// Mock rcl_init_options_copy to fail -TEST_F(CLASSNAME(TestRCLFixture, RMW_IMPLEMENTATION), test_rcl_init_copy_mocked_fail_fini) { - rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); - rcl_ret_t ret = rcl_init_options_init(&init_options, rcl_get_default_allocator()); - ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( - { - EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options)) << rcl_get_error_string().str; - }); - rcl_init_options_t init_options_dst = rcl_get_zero_initialized_init_options(); - auto mock = mocking_utils::inject_on_return("lib:rcl", rmw_init_options_fini, RMW_RET_ERROR); - EXPECT_EQ(RCL_RET_ERROR, rcl_init_options_copy(&init_options, &init_options_dst)); - rcl_reset_error(); - auto mock_ok = mocking_utils::inject_on_return("lib:rcl", rmw_init_options_fini, RMW_RET_OK); - EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options_dst)); -} - // Mock rcl_init_options_copy to fail TEST_F(CLASSNAME(TestRCLFixture, RMW_IMPLEMENTATION), test_rcl_init_options_copy_fail_rmw_copy) { rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); From 7ccbbe1f6213570149b62c015c7d3b50a47f2ccb Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Fri, 6 Nov 2020 09:48:29 +0800 Subject: [PATCH 3/3] use inline for the helper function and rename it to look more clear Co-authored-by: Chris Lalancette Signed-off-by: Chen Lihui --- rcl/src/rcl/init_options.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rcl/src/rcl/init_options.c b/rcl/src/rcl/init_options.c index 965d72178..5fa93c70d 100644 --- a/rcl/src/rcl/init_options.c +++ b/rcl/src/rcl/init_options.c @@ -35,9 +35,9 @@ rcl_get_zero_initialized_init_options(void) } /// Initialize given init_options with the default values and zero-initialize implementation. -RCL_LOCAL +static inline rcl_ret_t -_rcl_init_options_init(rcl_init_options_t * init_options, rcl_allocator_t allocator) +_rcl_init_options_zero_init(rcl_init_options_t * init_options, rcl_allocator_t allocator) { init_options->impl = allocator.allocate(sizeof(rcl_init_options_impl_t), allocator.state); RCL_CHECK_FOR_NULL_WITH_MSG( @@ -65,7 +65,7 @@ rcl_init_options_init(rcl_init_options_t * init_options, rcl_allocator_t allocat } RCL_CHECK_ALLOCATOR(&allocator, return RCL_RET_INVALID_ARGUMENT); - rcl_ret_t ret = _rcl_init_options_init(init_options, allocator); + rcl_ret_t ret = _rcl_init_options_zero_init(init_options, allocator); if (RCL_RET_OK != ret) { return ret; } @@ -96,7 +96,7 @@ rcl_init_options_copy(const rcl_init_options_t * src, rcl_init_options_t * dst) } // initialize dst (since we know it's in a zero initialized state) - rcl_ret_t ret = _rcl_init_options_init(dst, src->impl->allocator); + rcl_ret_t ret = _rcl_init_options_zero_init(dst, src->impl->allocator); if (RCL_RET_OK != ret) { return ret; // error already set }