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

rclcpp/context lacking a const method for shutdown_reason #1323

Closed
Blast545 opened this issue Sep 21, 2020 · 4 comments
Closed

rclcpp/context lacking a const method for shutdown_reason #1323

Blast545 opened this issue Sep 21, 2020 · 4 comments
Labels
help wanted Extra attention is needed

Comments

@Blast545
Copy link
Contributor

Adding coverage tests for context module (#1321), we have realized there is no const method for the shutdown_reason method. See link to the code

This issue is to track the conversation here and get some feedback.

@brawner
Copy link
Contributor

brawner commented Sep 21, 2020

Link to .cpp (

std::string
Context::shutdown_reason()
{
std::lock_guard<std::recursive_mutex> lock(init_mutex_);
return shutdown_reason_;
}
). This function requires locking the init_mutex_ which prevents it from being const. I suggested @Blast545 open this issue, just to see if others thought there might be more preferred ways of doing this.

One possible solution would be to make init_mutex_ and perhaps other mutexes mutable.

@Blast545 Blast545 removed their assignment Oct 1, 2020
@clalancette clalancette added the help wanted Extra attention is needed label Oct 15, 2020
@suab321321
Copy link
Contributor

suab321321 commented Feb 21, 2021

@clalancette should I follow @brawner and make it the init_mutex_ mutable?
and open a PR

@clalancette
Copy link
Contributor

@clalancette should I follow @brawner and make it the init_mutex_ mutable?
and open a PR

Yeah, that is the usual way to deal with this problem. Making init_mutex_ mutable seems totally reasonable to me.

@fujitatomoya
Copy link
Collaborator

done with #1578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants