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.

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

Fix that not to delete some objects after destroying functions #236

Merged
merged 4 commits into from
Sep 11, 2020

Conversation

iuhilnehc-ynos
Copy link

@iuhilnehc-ynos iuhilnehc-ynos commented Sep 4, 2020

Fixes #235

Signed-off-by: Chen.Lihui lihui.chen@sony.com

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>
rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved
rmw_cyclonedds_cpp/src/rmw_node.cpp Show resolved Hide resolved
rmw_cyclonedds_cpp/src/rmw_node.cpp Show resolved Hide resolved
@fujitatomoya
Copy link
Contributor

@iuhilnehc-ynos

besides, cppcheck looks unhappy for windows and macOS. could you look into it?

@iuhilnehc-ynos
Copy link
Author

@fujitatomoya

@iuhilnehc-ynos

besides, cppcheck looks unhappy for windows and macOS. could you look into it?

I encounter that before (in some previous PR), but It seems that maintainers not decide to deal with them.

Such as,

  [7.801s] 2: [src/rmw_node.cpp:704]: (error: unknownMacro) There is an unknown macro here somewhere. Configuration is required. If RCUTILS_STRINGIFY is a macro then please configure it.

RCUTILS_STRINGIFY(__FILE__) ":" RCUTILS_STRINGIFY(__function__) ":"

@eboasson
Copy link
Collaborator

eboasson commented Sep 6, 2020

@fujitatomoya

@iuhilnehc-ynos
besides, cppcheck looks unhappy for windows and macOS. could you look into it?

I encounter that before (in some previous PR), but It seems that maintainers not decide to deal with them.

It isn't being ignored so much as that cppcheck is disagreeing with correct code and fixing it turns out to not be so trivial (see #220).

And @iuhilnehc-ynos and @fujitatomoya thank you for all the work! I look forward to the final touches on the PR so that it can be merged.

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>
Co-authored-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@iuhilnehc-ynos
Copy link
Author

iuhilnehc-ynos commented Sep 7, 2020

@eboasson

It isn't being ignored so much as that cppcheck is disagreeing with correct code and fixing it turns out to not be so trivial (see #220).

Thanks for your information. I'll also keep my eyes on it and try to get a method to fix it.
(I don't know if ivanpauno will accept the solution about --suppress=unknownMacro)

@iuhilnehc-ynos
Copy link
Author

iuhilnehc-ynos commented Sep 7, 2020

@fujitatomoya

Could you review it again?
NOTE: Do you know why I can't use Re-request review link for you?
image

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

LGTM

@fujitatomoya
Copy link
Contributor

Do you know why I can't use Re-request review link for you?

No idea, sorry...

@iuhilnehc-ynos
Copy link
Author

iuhilnehc-ynos commented Sep 7, 2020

I don't know if we need to explicitly add rcutils path to '--include_dirs' for cppcheck.

  • debug log
chenlh@lihuideMacBook-Pro rmw_cyclonedds_cpp % cd /Users/chenlh/Projects/ROS2/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp

chenlh@lihuideMacBook-Pro rmw_cyclonedds_cpp % /Users/chenlh/Projects/ROS2/install/ament_cppcheck/bin/ament_cppcheck --xunit-file /Users/chenlh/Projects/ROS2/build/rmw_cyclonedds_cpp/test_results/rmw_cyclonedds_cpp/cppcheck.xunit.xml --include_dirs /Users/chenlh/Projects/ROS2/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include
[src/rmw_node.cpp:713]: (error: unknownMacro) There is an unknown macro here somewhere. Configuration is required. If RCUTILS_STRINGIFY is a macro then please configure it.
1 errors

chenlh@lihuideMacBook-Pro rmw_cyclonedds_cpp % /Users/chenlh/Projects/ROS2/install/ament_cppcheck/bin/ament_cppcheck --xunit-file /Users/chenlh/Projects/ROS2/build/rmw_cyclonedds_cpp/test_results/rmw_cyclonedds_cpp/cppcheck.xunit.xml --include_dirs /Users/chenlh/Projects/ROS2/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include /Users/chenlh/Projects/ROS2/src/ros2/rcutils/include
No problems found
  • patch as following,
diff --git a/rmw_cyclonedds_cpp/CMakeLists.txt b/rmw_cyclonedds_cpp/CMakeLists.txt
index 480f1ac..08b0da0 100644
--- a/rmw_cyclonedds_cpp/CMakeLists.txt
+++ b/rmw_cyclonedds_cpp/CMakeLists.txt
@@ -105,6 +105,7 @@ register_rmw_implementation(

 if(BUILD_TESTING)
   find_package(ament_lint_auto REQUIRED)
+  set(ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS ${rcutils_INCLUDE_DIRS})
   ament_lint_auto_find_test_dependencies()
 endif()

@iuhilnehc-ynos
Copy link
Author

  • macOS
chenlh@lihuideMacBook-Pro rmw_cyclonedds % /usr/local/bin/cppcheck --version
Cppcheck 2.1
chenlh@lihuideMacBook-Pro rmw_cyclonedds % /usr/local/bin/cppcheck -f --inline-suppr -q -rp --suppress=internalAstError -I /Users/chenlh/Projects/ROS2/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include rmw_cyclonedds_cpp/src/rmw_node.cpp
rmw_cyclonedds_cpp/src/rmw_node.cpp:713:41: error: There is an unknown macro here somewhere. Configuration is required. If RCUTILS_STRINGIFY is a macro then please configure it. [unknownMacro]
        RCUTILS_STRINGIFY(__FILE__) ":" RCUTILS_STRINGIFY(__function__) ":"
  • ubuntu
chenlh@OptiPlex-7050:~/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds$ /usr/bin/cppcheck --version
Cppcheck 1.90
chenlh@OptiPlex-7050:~/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds$ /usr/bin/cppcheck -f --inline-suppr -q -rp  --suppress=internalAstError -I /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include rmw_cyclonedds_cpp/src/rmw_node.cpp
chenlh@OptiPlex-7050:~/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds$ 

Next step, I think I need to check cppcheck not ament_cppcheck

@iuhilnehc-ynos
Copy link
Author

iuhilnehc-ynos commented Sep 8, 2020

After using cppcheck(2.1) at ubuntu, it failed with the same error(unknownMacro), I believe that using concrete "-I path" for cppcheck is a feature of v2.1 compared with v1.9. (Sorry about not looking into cppcheck source.) So the patch about using 'set(ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS ${rcutils_INCLUDE_DIRS})' in CMakeLists.txt seems one solution. (Why not updating ament_lint_auto_find_test_dependencies to add all INCLUDE_DIRS for ament_cppcheck automatically at internal? ament/ament_lint#265 is a better idea.)

About unknownMacro issue, it's better to fix on #220 .

@clalancette
Copy link
Contributor

After using cppcheck(2.1) at ubuntu, it failed with the same error(unknownMacro), I believe that using concrete "-I path" for cppcheck is a feature of v2.1 compared with v1.9. (Sorry about not looking into cppcheck source.)

Note that we have an overall problem with cppcheck 2.1: ros2/ros2#942 . Because of that, we are currently pinned to cppcheck 1.9 on all platforms.

@iuhilnehc-ynos
Copy link
Author

pinned to cppcheck 1.9

That's good to know.

@iuhilnehc-ynos
Copy link
Author

@eboasson

I look forward to the final touches on the PR so that it can be merged.

Could you please review it? (I hope you got a happy mood.)

Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

Hi @iuhilnehc-ynos there are three things:

  • I am embarrassed by the number of memory leaks
  • In the long run, it would probably be worth it to rely more heavily on unique_ptrs
  • And there's one thing I suggest changing in the PR, it doesn't actually fix anything but I think it may help in the future.

Thank you!

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved
Signed-off-by: Chen.Lihui <lihui.chen@sony.com>
Co-authored-by: eboasson <eb@ilities.com>
@iuhilnehc-ynos
Copy link
Author

@eboasson

In the long run, it would probably be worth it to rely more heavily on unique_ptrs

I used the unique_ptr if it could be.

there's one thing I suggest changing in the PR

After using unique_ptr, it seems not necessary to change with the suggestion.

Could you please review it again? Thank you.

Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

@iuhilnehc-ynos it looks good to me, thank you! @ivanpauno or @hidmic I'd appreciate it if you could take a final look because I don't quite trust myself with C++ just yet ...

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

lgtm with one minor question

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved
Signed-off-by: Chen.Lihui <lihui.chen@sony.com>
@ivanpauno
Copy link
Member

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 4a5b645 into ros2:master Sep 11, 2020
eboasson pushed a commit to eboasson/rmw_cyclonedds that referenced this pull request May 17, 2021
)

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cdds[Client/Service] created in rmw_create_[client/service] not destroyed after rmw_destroy_[client/service]
5 participants