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 leak(#480) #481

Merged
merged 2 commits into from
Jan 19, 2021
Merged

Fix leak(#480) #481

merged 2 commits into from
Jan 19, 2021

Conversation

y-okumura-isp
Copy link
Contributor

Fix #480.
Although there are ASAN errors related to class_loader, this PR reduces one error.

Signed-off-by: y-okumura-isp y-okumura@isp.co.jp

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One nit, and then I'm happy to approve and run CI.

@@ -32,7 +32,7 @@ int main(int argc, char * argv[])
rclcpp::Logger logger = rclcpp::get_logger(LINKTIME_COMPOSITION_LOGGER_NAME);
rclcpp::executors::SingleThreadedExecutor exec;
rclcpp::NodeOptions options;
std::vector<class_loader::ClassLoader *> loaders;
std::vector<std::shared_ptr<class_loader::ClassLoader>> loaders;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is only ever going to be one owner, I would suggest using a std::unique_ptr here.

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
@y-okumura-isp
Copy link
Contributor Author

y-okumura-isp commented Jan 18, 2021

Thank you for comment. I updated PR.
Sorry for late reply as I studied why we should have loaders.

http://wiki.ros.org/class_loader may be the reason.

2.5 Understanding Loading and Unloading
if I create an object from a class defined within a runtime library, I then unload the library, and then I try to use the object...things will go bad.

@clalancette
Copy link
Contributor

CI:

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

@clalancette
Copy link
Contributor

Windows CI is failing for a completely unrelated reason. I think this is safe enough to put it without that, so I'm going to go ahead and merge.

@clalancette clalancette merged commit a66f0e8 into ros2:master Jan 19, 2021
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.

linktime_composition ASAN LeakSanitizer error
2 participants