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

Better log output when nodelet initialization fails #98

Open
wants to merge 3 commits into
base: noetic-devel
Choose a base branch
from

Conversation

stertingen
Copy link

This missing output of the initialization failure gave me a hard time debugging an rqt plugin.

Such errors should be visible without debug output turned on.

Furthermore, I noticed a slightly difference in code style (space before macro parenthesis, parenthesis around return value), which was also fixed.

@@ -322,13 +322,13 @@ bool Loader::load(const std::string &name, const std::string& type, const ros::M


ROS_DEBUG("Done initing nodelet %s", name.c_str());
} catch(...) {
} catch(std::runtime_error& e) {
Copy link

Choose a reason for hiding this comment

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

what about other possible exceptions (although probably not likely)?
Maybe catch std::exception instead?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, but in this file, std::runtime_error is caught the same way in another location. If I changed it there, I'd have to change it at the other location for the sake of consistency.

The ultimately best way would be:

try {
    //
} catch (const std::exception& e) {
    // print error with e.what()
} catch (...} {
    // print error without e.what() in case something not derived from std::exception was thrown
}

Copy link
Author

Choose a reason for hiding this comment

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

Changed both to const std::exception& now, but without the default catch.

@stertingen stertingen force-pushed the fix_initialize_error_handling branch from 657c8e7 to 43d7b92 Compare January 13, 2020 16:59
Also, catch const reference instead of non-const reference
@stertingen stertingen force-pushed the fix_initialize_error_handling branch from 43d7b92 to b96daf2 Compare January 13, 2020 17:03
@stertingen stertingen changed the base branch from indigo-devel to noetic-devel May 18, 2020 16:58
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.

2 participants