Private strings: use of const
A few months ago we had a thread about static code analysis with coverity. However, I have not heard anything from it anymore. So I just installed myself another static code analysis software packet, cppcheck. I played with it a bit and stumbled upon several notifications of functions that could be const. See e.g. the extract of the log:
[E:\Inkscapecode\inkscape\src\extension\extension.h:122]: (style) The function 'Extension::get_help' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:111]: (style) The function 'Parameter::name' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:116]: (style) The function 'Parameter::get_tooltip' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:119]: (style) The function 'Parameter::get_gui_hidden' can be const [E:\Inkscapecode\inkscape\src\extension\dbus\dbus-init.cpp:87]: (style) Unused variable: obj [E:\Inkscapecode\inkscape\src\extension\dbus\dbus-init.cpp:104]: (style) Variable 'error' is assigned a value that is never used
So the first item lists line 112 of extension.h: gchar const * get_help (void) { return _help; }
which should return the pointer to a string in a private variable.
However what should be const? The pointer, the string to which the pointer points to or both? Aparently the pointer should be const, no? (e.g. to ensure that the string can not be deleted in some buggy code that calls get_help, leaving the pointer_help in the class pointing to some odd data chunk?)
This would result in: gchar * const get_help (void) { return _help; }
Or is there something that I am missing? BTW: this is just an example, the same reasoning is valid for the listings in parameter.h
Regards Kris
Forther my previous mail. It was already late and I misread something when reading http://www.possibility.com/Cpp/const.html Something like: gchar const * const get_help (void) { return _help; }
would also be overkill is not it, and it would also break compilation
2010/9/19 Kris De Gussem <kris.degussem@...400...>:
A few months ago we had a thread about static code analysis with coverity. However, I have not heard anything from it anymore. So I just installed myself another static code analysis software packet, cppcheck. I played with it a bit and stumbled upon several notifications of functions that could be const. See e.g. the extract of the log:
[E:\Inkscapecode\inkscape\src\extension\extension.h:122]: (style) The function 'Extension::get_help' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:111]: (style) The function 'Parameter::name' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:116]: (style) The function 'Parameter::get_tooltip' can be const [E:\Inkscapecode\inkscape\src\extension\param/parameter.h:119]: (style) The function 'Parameter::get_gui_hidden' can be const [E:\Inkscapecode\inkscape\src\extension\dbus\dbus-init.cpp:87]: (style) Unused variable: obj [E:\Inkscapecode\inkscape\src\extension\dbus\dbus-init.cpp:104]: (style) Variable 'error' is assigned a value that is never used
So the first item lists line 112 of extension.h: gchar const * get_help (void) { return _help; }
which should return the pointer to a string in a private variable.
However what should be const? The pointer, the string to which the pointer points to or both? Aparently the pointer should be const, no? (e.g. to ensure that the string can not be deleted in some buggy code that calls get_help, leaving the pointer_help in the class pointing to some odd data chunk?)
This would result in: gchar * const get_help (void) { return _help; }
Or is there something that I am missing? BTW: this is just an example, the same reasoning is valid for the listings in parameter.h
Regards Kris
On Sep 19, 2010, at 1:26 PM, Kris De Gussem wrote:
So the first item lists line 112 of extension.h: gchar const * get_help (void) { return _help; }
which should return the pointer to a string in a private variable.
Well, first I'd change things away from the ancient GNU formatting. That was helpful in the days before editors could understand code, but those days are long since past.
gchar const *get_help(void) {return _help; }
Now we see that "get_help" is the function, and that it takes no pointer. We also get to see that nothing should go between "get_help" and "(void)".
We also have adopted an approach for ordering const and such that helps things be more legible. Keep in mind that for references and const one should read from right-to-left. Given that, the function returns "a pointer to constant gchar data." The hint from the tool is that the *method* itself can be const
gchar const *get_help(void) const {return _help;}
Note how spacing things out properly made it a little easier to see how something could go in right after the "(void)". Also what it means to make the method const is that it marks it as a method that does not functionally change the object it is called on, and can be called via a const pointer to that type of object.
*HOWEVER* we probably *shouldn't* do that.
For that, we need to use some human-level reasoning. What is that _help member that we are returning? It is declared as "private gchar* _help;" So the first problem is that we are returning an internal detail. That is, we have a pointer that the class owns, but we return that directly. There are many reasons this could be an issue, but the first one to consider for a pointer is lifecycle. Who owns that pointer? How long will it be valid? Is it possible that someone who calls "get_help()" might hold onto that pointer longer than it stays valid? (short answer: "yes")
Luckily C++ made passing around objects much easier. So to gain safety what we *should* end up with is changing
gchar *_help;
to instead be
Glib::ustring _help;
and then
gchar const *get_help(void) {return _help; }
to be
Glib::ustring get_help(void) const {return _help; }
That address both the const issue and corrects the dangerous passing of an internal pointer.
The bottom line we have here is that the tools can be helpful to point out places to look, but that we *really* need to look carefully at both the how and the why of the code being examined. Generally we want to do smarter fixes in the code that needs to be touched. Often doing the higher-level cleanup will lead to the low-level detail also being fixed.
On Sep 19, 2010, at 1:26 PM, Kris De Gussem wrote:
Or is there something that I am missing? BTW: this is just an example, the same reasoning is valid for the listings in parameter.h
Sorry for the bit of distraction on that last response. I did spot a key thing you were missing in most of these warnings.
Start by thinking Object-Oriented. Say we have an "Extension" object. Often we have one of these
Extension const *ptr = getSomeComplexThing();
So we have a pointer to an object that we're not allowed to change. A const method is a method that won't change an object it is called on. To mark a method const, simple place "const" at the end of the method declaration, and before any body if present.
gchar const *get_help() const;
That takes care of the "how". Now to get on to the "why" part in play here. One thing I found through most of those methods is that they all start with "get". That is a clue. The methods are accessors (or "gettors"). That is, they give a way to read a property of an object. Usually accessors are const so that one can read safely from any object.
So in general what that tool is saying matches up with our concept of an OO accessor. Those methods that are accessors should be const methods. (in a similar vein input parameters that are pointers or references should also be const).
2010/9/19 Kris De Gussem <kris.degussem@...400...>:
This would result in: gchar * const get_help (void) { return _help; }
Or is there something that I am missing? BTW: this is just an example, the same reasoning is valid for the listings in parameter.h
Condensed reply: The warning tells you to mark the function as const, so it can be called when you use a constant reference or a constant pointer to the object. It's not related to const return types or const parameters.
Here's some more information: http://msdn.microsoft.com/en-us/library/6ke686zh%28VS.80%29.aspx
Regards, Krzysztof
participants (3)
-
Jon Cruz -
Kris De Gussem -
Krzysztof Kosiński