Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor for removing unnecessary source code #857

Merged
merged 3 commits into from
Nov 6, 2020
Merged

refactor for removing unnecessary source code #857

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f7f8b2
refactor for removing unnecessary source code
Nov 5, 2020
4fcec36
Add a helper function that make rcl_init_options_copy not to call rmw…
Nov 5, 2020
7ccbbe1
use inline for the helper function and rename it to look more clear
Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 24 additions & 28 deletions rcl/src/rcl/init_options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_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)

(or something like that, just to distinguish it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your quick response and the suggestion.

I have done valgrind for this file by valgrind --leak-check=full ./test_init__rmw_fastrtps_cpp, and got the same result that not leaking memory as the commit of the master branch.

valgrind log 

[----------] Global test environment tear-down
[==========] 11 tests from 1 test suite ran. (418 ms total)
[  PASSED  ] 11 tests.
==2880876== 
==2880876== HEAP SUMMARY:
==2880876==     in use at exit: 232 bytes in 5 blocks
==2880876==   total heap usage: 2,673 allocs, 2,668 frees, 987,584 bytes allocated
==2880876== 
==2880876== LEAK SUMMARY:
==2880876==    definitely lost: 0 bytes in 0 blocks
==2880876==    indirectly lost: 0 bytes in 0 blocks
==2880876==      possibly lost: 0 bytes in 0 blocks
==2880876==    still reachable: 232 bytes in 5 blocks
==2880876==         suppressed: 0 bytes in 0 blocks
==2880876== Reachable blocks (those to which a pointer was found) are not shown.
==2880876== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==2880876== 
==2880876== For lists of detected and suppressed errors, rerun with: -s
==2880876== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you so much for doing that additional test. It just gives us a bit more confidence that we didn't miss something.

{
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)
{
Expand All @@ -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);
Expand All @@ -74,39 +88,21 @@ 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");
return RCL_RET_ALREADY_INIT;
}

// 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
}

// 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();
Expand Down
17 changes: 0 additions & 17 deletions rcl/test/rcl/test_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down