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.