-
Notifications
You must be signed in to change notification settings - Fork 155
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
Proper return value after assert #83
Conversation
@pbeeson I reenabled the asserts here but printed proper error messages and return values as per your original fix. If you want to remove these asserts you'll need to use the binaries or compile your code with the build type "Debug". @ros/ros_team This being a change in behavior for the released packages I'd like a review + feedback on the distributions it should be released into. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the changes "fix" undefined behavior I don't see a reason not to release it inot all ROS distros.
if (!gm_) { | ||
ROS_ERROR_NAMED("actionlib", "Client should have valid GoalManager"); | ||
return CommState(CommState::DONE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If gm_
is being checked that should happen at the beginning of the function. Otherwise line 94 would already 💥
include/actionlib/managed_list.h
Outdated
@@ -154,12 +154,16 @@ class ManagedList | |||
T& getElem() | |||
{ | |||
assert(valid_); | |||
if (!valid_) | |||
ROS_ERROR_NAMED("actionlib", "getElem() should not see non-valid handles"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use { ... }
. Otherwise the logic is broken if the macro is being defined as empty.
Arguably raising an exception might be more appropriate here.
Same below.
Nitpick: non-valid
-> invalid
.
{ | ||
ROS_ERROR_NAMED("actionlib", "execute_thread_ is null, skipping attempt to join it"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a case which shouldn't be possible based on the logic of the class. An assert which is also effective in release builds would be sufficient imo.
It could also be changed to:
if (execute_thread_) {
execute_thread_->join();
delete execute_thread_;
execute_thread_ = NULL;
}
thanks for the feedback! Given that nothing throws in this code I didn't add exceptions for now. I think I address every other comment |
thanks @pbeeson for reporting the issue and contributing a fix |
Thanks for handling this.
…On Mon, Jul 17, 2017 at 7:15 PM Mikael Arguedas ***@***.***> wrote:
thanks @pbeeson <https://github.com/pbeeson> for reporting the issue and
contributing a fix
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#83 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADrfxviwOvGBs4uwiV3c4s6XtJvIEMV7ks5sO_VCgaJpZM4OTbCn>
.
|
In release mode these asserts are removed and the code keeps going through normal codepath, it should print errors and return values accordingly.
Replaces #80
Connects to #67