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

address bugprone-exception-escape warning from clang-tidy. #1894

Conversation

fujitatomoya
Copy link
Collaborator

draft for #1890

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
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!

Did you verify this solves #1890?

@fujitatomoya
Copy link
Collaborator Author

@ivanpauno no, im not sure how to do that... @alsora could i have some help here? or could you tell me how to verify?

@@ -37,7 +37,15 @@ int main(int argc, char * argv[])
std::string class_name = "rclcpp_components::NodeFactoryTemplate<@component@>";

RCLCPP_DEBUG(logger, "Load library %s", library_name.c_str());
auto loader = new class_loader::ClassLoader(library_name);
try {
auto loader = new class_loader::ClassLoader(library_name);
Copy link
Collaborator

@alsora alsora Mar 7, 2022

Choose a reason for hiding this comment

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

this code does not compile.
you should have
class_loader::ClassLoader * loader = nullptr;
outside of the try catch otherwise you will can't access it later

Copy link
Collaborator

Choose a reason for hiding this comment

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

slightly unrelated, but is there a reason why we use a raw pointer here?
this is not even deallocated at the end of the file (not a big deal, but still bad to see).

Copy link
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

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

The code does not compile as it is.
However, with a minimal change (described above) it does and the try catch block fixes the bugprone warning.

@alsora
Copy link
Collaborator

alsora commented Mar 7, 2022

Sorry, I made a mistake. The error is still there even after your patch.
It looks like this is the function throwing an exception https://github.com/ros/class_loader/blob/galactic/include/class_loader/class_loader_core.hpp#L316

@alsora
Copy link
Collaborator

alsora commented Mar 7, 2022

@fujitatomoya I stole your try-catch block and I opened a PR that fixes the issue: #1895

@fujitatomoya
Copy link
Collaborator Author

@alsora thanks! i will review #1895 instead.

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.

None yet

3 participants