Merged
Conversation
rcl/src/rcl/wait.c
Outdated
Member
There was a problem hiding this comment.
Why not declare the ret variable in each local scope since the variable seems to never be used in this bigger scope?
Also shouldn't all the checks with assert also be done when building release?
Member
Author
There was a problem hiding this comment.
No particular reason, but I don't see an advantage to changing that.
As each of the asserts are commented those are defensive checks, so I'd say they don't need to be checked at runtime. I'm sort of agnostic one way or the other.
Member
There was a problem hiding this comment.
I am argueing for the smaller scope based on our programming conventions (https://github.com/ros2/ros2/wiki/Developer-Guide#programming-conventions):
Declare variables in the narrowest scope possible.
If you think the checks are fine for debug builds only that is ok for me.
Member
Author
There was a problem hiding this comment.
Well, I interpret that guideline as particularly for a scenario like this:
// This way:
int foo()
{
{
int i;
i = 3;
}
}
// Rather than this way:
int foo()
{
int i;
{
i = 3;
}
}But in this scenario we're actually reusing the variable across multiple scopes. But since we're not getting any advantage with this (other than less code) I changed it in 3310359.
Member
There was a problem hiding this comment.
Well, the variables in the different scopes only have the same name but are never used to pass information. They could easily have unique names. Therefore I think the smallest scope rule applies.
Member
|
Lgtm |
emersonknapp
pushed a commit
to aws-ros-dev/rcl
that referenced
this pull request
Jun 3, 2019
* initial move of error_handling from rmw * initial move of allocator from rcl * refactor allocator implementation * refactor error_handling * leave gtest where it was being used before * suppress internal gmock warnings * proctect call to set_target_properties * avoid segfault * fix whitespace in cmake * fix doc * comment * fix error string logic and check deallocate before using * typo * simplify logic in CMake * remove redundant check and free
emersonknapp
pushed a commit
to aws-ros-dev/rcl
that referenced
this pull request
Jun 3, 2019
Call rcutils_logging_shutdown() in TearDown
to avoid leaking logging internal objects.
As any change to this file makes test_logging_named
fail, this releaxes the EXACT_EQ call on
g_last_log_event.location->line_number[1]. Instead
of looking for a particular value, we test an invariant
of the field (line numbers are positive).
Finally, in test_logging_function, initialize g_counter
and g_function_called values to make sure the test will
not fail even if `--gtest_repeat=2` is passed.
Other tests are still failing when this flag is passed:
[ FAILED ] TestLoggingMacros.test_logging_once
[ FAILED ] TestLoggingMacros.test_logging_skipfirst
[ FAILED ] TestLoggingMacros.test_logging_skipfirst_throttle
However, fixing those tests would require actually changing
the logging implementation which is out of the scope of this
change.
Before:
$ ./test_logging_macros
Running main() from gmock_main.cc
[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from TestLoggingMacros
[ RUN ] TestLoggingMacros.test_logging_named
[ OK ] TestLoggingMacros.test_logging_named (0 ms)
[ RUN ] TestLoggingMacros.test_logging_once
[ OK ] TestLoggingMacros.test_logging_once (0 ms)
[ RUN ] TestLoggingMacros.test_logging_expression
[ OK ] TestLoggingMacros.test_logging_expression (0 ms)
[ RUN ] TestLoggingMacros.test_logging_function
[ OK ] TestLoggingMacros.test_logging_function (0 ms)
[ RUN ] TestLoggingMacros.test_logging_skipfirst
[ OK ] TestLoggingMacros.test_logging_skipfirst (0 ms)
[ RUN ] TestLoggingMacros.test_logging_throttle
[ OK ] TestLoggingMacros.test_logging_throttle (302 ms)
[ RUN ] TestLoggingMacros.test_logging_skipfirst_throttle
[ OK ] TestLoggingMacros.test_logging_skipfirst_throttle (302 ms)
[ RUN ] TestLoggingMacros.test_logger_hierarchy
[ OK ] TestLoggingMacros.test_logger_hierarchy (0 ms)
[----------] 8 tests from TestLoggingMacros (605 ms total)
[----------] Global test environment tear-down
[==========] 8 tests from 1 test case ran. (605 ms total)
[ PASSED ] 8 tests.
=================================================================
==19903==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 504 byte(s) in 7 object(s) allocated from:
#0 0x7fd640c1fb50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
#1 0x7fd64091cbb5 in __default_allocate (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5)
#2 0x7fd64092f1b0 in rcutils_string_map_init (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0)
#3 0x7fd640926641 in rcutils_logging_initialize_with_allocator (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641)
#4 0x7fd6409261ab in rcutils_logging_initialize (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab)
#5 0x564405fc6627 in TestLoggingMacros::SetUp() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x29627)
#6 0x564406045999 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0xa8999)
#7 0x564406037b7d in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x9ab7d)
#8 0x564405fe4503 in testing::Test::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x47503)
#9 0x564405fe59e4 in testing::TestInfo::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x489e4)
ros2#10 0x564405fe6588 in testing::TestCase::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x49588)
ros2#11 0x564406001699 in testing::internal::UnitTestImpl::RunAllTests() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x64699)
ros2#12 0x56440604844c in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0xab44c)
ros2#13 0x564406039e46 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x9ce46)
ros2#14 0x564405ffe42d in testing::UnitTest::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x6142d)
ros2#15 0x564405fd197d in RUN_ALL_TESTS() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x3497d)
ros2#16 0x564405fd18c3 in main (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x348c3)
ros2#17 0x7fd63fd84b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
After:
$ ./test_logging_macros
Running main() from gmock_main.cc
[==========] Running 9 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 9 tests from TestLoggingMacros
[ RUN ] TestLoggingMacros.test_empty
[ OK ] TestLoggingMacros.test_empty (0 ms)
[ RUN ] TestLoggingMacros.test_logging_named
[ OK ] TestLoggingMacros.test_logging_named (0 ms)
[ RUN ] TestLoggingMacros.test_logging_once
[ OK ] TestLoggingMacros.test_logging_once (0 ms)
[ RUN ] TestLoggingMacros.test_logging_expression
[ OK ] TestLoggingMacros.test_logging_expression (0 ms)
[ RUN ] TestLoggingMacros.test_logging_function
[ OK ] TestLoggingMacros.test_logging_function (0 ms)
[ RUN ] TestLoggingMacros.test_logging_skipfirst
[ OK ] TestLoggingMacros.test_logging_skipfirst (0 ms)
[ RUN ] TestLoggingMacros.test_logging_throttle
[ OK ] TestLoggingMacros.test_logging_throttle (303 ms)
[ RUN ] TestLoggingMacros.test_logging_skipfirst_throttle
[ OK ] TestLoggingMacros.test_logging_skipfirst_throttle (302 ms)
[ RUN ] TestLoggingMacros.test_logger_hierarchy
[ OK ] TestLoggingMacros.test_logger_hierarchy (0 ms)
[----------] 9 tests from TestLoggingMacros (605 ms total)
[----------] Global test environment tear-down
[==========] 9 tests from 1 test case ran. (606 ms total)
[ PASSED ] 9 tests.
[1] https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
emersonknapp
pushed a commit
to aws-ros-dev/rcl
that referenced
this pull request
Jun 3, 2019
Add rcutils_logging_shutdown() every time needed
in test_logging.cpp. Remove spurious `g_rcutils_logging_initialized = false;`
preventing rcutils_logging_shutdown() from freeing memory.
Before:
$ ./test_logging
Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from TestLogging
[ RUN ] TestLogging.test_logging_initialization
[ OK ] TestLogging.test_logging_initialization (0 ms)
[ RUN ] TestLogging.test_logging
[ OK ] TestLogging.test_logging (0 ms)
[ RUN ] TestLogging.test_log_severity
[ OK ] TestLogging.test_log_severity (0 ms)
[ RUN ] TestLogging.test_logger_severities
[ OK ] TestLogging.test_logger_severities (0 ms)
[ RUN ] TestLogging.test_logger_severity_hierarchy
[ OK ] TestLogging.test_logger_severity_hierarchy (0 ms)
[----------] 5 tests from TestLogging (0 ms total)
[----------] Global test environment tear-down
[==========] 5 tests from 1 test case ran. (0 ms total)
[ PASSED ] 5 tests.
=================================================================
==1676==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 72 byte(s) in 1 object(s) allocated from:
#0 0x7f7a7229db50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
#1 0x7f7a71f9abb5 in __default_allocate (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5)
#2 0x7f7a71fad1b0 in rcutils_string_map_init (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0)
#3 0x7f7a71fa4641 in rcutils_logging_initialize_with_allocator (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641)
#4 0x7f7a71fa41ab in rcutils_logging_initialize (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab)
#5 0x561e9d22c972 in TestLogging_test_logging_Test::TestBody() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x1a972)
#6 0x561e9d2b545d in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa345d)
#7 0x561e9d2a71c7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x951c7)
#8 0x561e9d2534c9 in testing::Test::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x414c9)
#9 0x561e9d2548f4 in testing::TestInfo::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x428f4)
ros2#10 0x561e9d255498 in testing::TestCase::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x43498)
ros2#11 0x561e9d2705a9 in testing::internal::UnitTestImpl::RunAllTests() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5e5a9)
ros2#12 0x561e9d2b7f10 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa5f10)
ros2#13 0x561e9d2a9490 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x97490)
ros2#14 0x561e9d26d33d in testing::UnitTest::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5b33d)
ros2#15 0x561e9d24088c in RUN_ALL_TESTS() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e88c)
ros2#16 0x561e9d2407d2 in main (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e7d2)
ros2#17 0x7f7a71402b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
Direct leak of 72 byte(s) in 1 object(s) allocated from:
#0 0x7f7a7229db50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
#1 0x7f7a71f9abb5 in __default_allocate (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5)
#2 0x7f7a71fad1b0 in rcutils_string_map_init (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0)
#3 0x7f7a71fa4641 in rcutils_logging_initialize_with_allocator (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641)
#4 0x7f7a71fa41ab in rcutils_logging_initialize (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab)
#5 0x561e9d22a9b6 in TestLogging_test_logging_initialization_Test::TestBody() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x189b6)
#6 0x561e9d2b545d in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa345d)
#7 0x561e9d2a71c7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x951c7)
#8 0x561e9d2534c9 in testing::Test::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x414c9)
#9 0x561e9d2548f4 in testing::TestInfo::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x428f4)
ros2#10 0x561e9d255498 in testing::TestCase::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x43498)
ros2#11 0x561e9d2705a9 in testing::internal::UnitTestImpl::RunAllTests() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5e5a9)
ros2#12 0x561e9d2b7f10 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa5f10)
ros2#13 0x561e9d2a9490 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x97490)
ros2#14 0x561e9d26d33d in testing::UnitTest::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5b33d)
ros2#15 0x561e9d24088c in RUN_ALL_TESTS() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e88c)
ros2#16 0x561e9d2407d2 in main (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e7d2)
ros2#17 0x7f7a71402b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
SUMMARY: AddressSanitizer: 144 byte(s) leaked in 2 allocation(s).
After:
$ ./test_logging
Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
[==========] Running 5 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 5 tests from TestLogging
[ RUN ] TestLogging.test_logging_initialization
[ OK ] TestLogging.test_logging_initialization (0 ms)
[ RUN ] TestLogging.test_logging
[ OK ] TestLogging.test_logging (0 ms)
[ RUN ] TestLogging.test_log_severity
[ OK ] TestLogging.test_log_severity (0 ms)
[ RUN ] TestLogging.test_logger_severities
[ OK ] TestLogging.test_logger_severities (0 ms)
[ RUN ] TestLogging.test_logger_severity_hierarchy
[ OK ] TestLogging.test_logger_severity_hierarchy (0 ms)
[----------] 5 tests from TestLogging (1 ms total)
[----------] Global test environment tear-down
[==========] 5 tests from 1 test case ran. (1 ms total)
[ PASSED ] 5 tests.
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should address #15 as well as a few other warnings I noticed.