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

PoseGraphInterface can mostly be const #1021

Closed
gaschler opened this issue Mar 27, 2018 · 4 comments · Fixed by #1156
Closed

PoseGraphInterface can mostly be const #1021

gaschler opened this issue Mar 27, 2018 · 4 comments · Fixed by #1156
Assignees

Comments

@gaschler
Copy link
Contributor

Currently, all methods in PoseGraphInterface are not const, even though most of them can be.
I think the reason was that they require a read mutex, but semantically, they are const.

@cschuet
Copy link

cschuet commented Apr 24, 2018

Making the mutex mutable is not forbidden by the C++ style guide

@SirVer
Copy link
Contributor

SirVer commented Apr 24, 2018

That is a philosophical decision. We decided way back that in Cartographer, const should mean the same for the compiler and us, so we never used mutable ever. mutable is also a slippery slope, diluting the possibility to correctly reason about class state from the outside. I think the Chromium project landed on the other side, using mutable all over the place and trying to express with const semantic constness.

Further reading, though a bit outdated:

@ojura
Copy link
Contributor

ojura commented Apr 24, 2018

I stumbled into this talk, which I found interesting. It argues that the meaning of const and mutable changed profoundly in C++11.
https://channel9.msdn.com/posts/C-and-Beyond-2012-Herb-Sutter-You-dont-know-blank-and-blank

@sebastianklose
Copy link
Contributor

@ojura - this is a nice talk, thanks for sharing.

For those who can't spare the time to watch it, the summary is that in C+11 the meaning of const and mutable has change away from "logically const" to "thread-safe" (bitwise const or internally synchronized). Meaning, declaring a synchronized object (like a mutex) mutable is concise and actually desired.

That being said, I will move forward with changing Cartographer classes to reflect this, as it also makes other classes using these object more sane in terms of const-ness.

@sebastianklose sebastianklose self-assigned this May 15, 2018
sebastianklose added a commit that referenced this issue May 17, 2018
Making getters and non-modifying methods  in PoseGraph* const.

Before most getters where marked non-const because of the need to acquire the mutex for thread safety. This forces all code using the PoseGraph to pass it around as a non-const object even if the consumer is not altering the object.
By making the mutexes mutable we can make getters and methods that do not change the PoseGraph const and thus enable consumer APIs to express how they are going to use the PoseGraph as well.

fixes #1021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants