Re: const fixes seek commit approval

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <rhpennin@midway.uchicago.edu> writes:

> The patch had a bug with USE_XIM defined, I removed a variable that was
> only sometimes unused in gtktext.c.
> 
> With that fixed, I built gnome-libs, gnome-core, gnome-utils, gnumeric,
> balsa, and gnome-media, all without problems. So there is no breakage, or
> at least not much.  Gtk+ itself compiles with only two warnings caused by
> the patch; these are in gtkfontsel.c, and I am not sure of the best way to
> fix them. They are harmless in any case. 

By 'no breakage' - are you saying that there are no
additional warnings at all, or just that it did compile.

The problem with 'const' returns, is that what initially looks
like a few innocuous warnings often can be solved either
with casts (and casts are much worse than the absence of
const, since they remove the ability of the compiler to
notice any discrepancy in types), or with hundreds of lines
of modifications to other parts of the code.
 
> I thought about the const-return thing a little more. Basically we are
> saying that:
> 
> [const] gchar* gtk_foo() { return "static string"; }
> 
> gchar* text = gtk_foo(); 
> 
> should not produce a warning.
> 
> Here is why I think this is wrong: if you are caring about warnings, for
> example, a goal for you is to compile your program without warnings, then
> you presumably want to use the compiler to find as many bugs as possible. 
> You are already getting many far less consequential warnings, like "unused
> variable" etc. Refusing to fix this particular warning seems kind of
> random. I don't see why someone would want to do this or why we should
> adjust to them if they do.
> 
> Not using const is fundamentally *wrong*, because this is a read-only
> pointer. We are lying about the type of the object. You can't modify a
> string literal; the results are undefined.

char *some_string;

Does _not_ mean "some modifiable string" - it means some string
with indeterminate modifiability. So I would disagree with the
statement that returning 'char *' is lying - it is merely
giving incomplete information.
 
> So you are punishing the people who are doing things the right way,
> because some people don't want to be pestered with a warning. 

'punishing' is a strong term here - they perhaps don't get every
possible benefit they could get, but they are not forced to
add extra casts, or to give up own use of const.

'punishment' is perhaps more of an apt term for the poor programmer
who is trying to mix use of an old non-const-correct interface,
with a interface that returns 'const'. (I've been there,
it isn't fun.)

> I don't
> think turning off -Wall or tossing in a (gchar*) cast or just enduring the
> warning is going to kill those people, if they insist on doing this for
> whatever reason. It's not like their program won't compile. The fact is
> that doing something dubious is supposed to result in a warning, and this
> is in fact something dubious.

Turning of -Wall is obviously not an option, enduring the warning
not much of an option either.
 
> Note that you are not avoiding the cast by not using const; you are just
> moving it into the library and making it implicit

That sounds a bit odd - most of the cases, the string is 
not constant in view the widget, so you in fact need a
(const char *) cast on the return to make it const...

>  so people can forget
> about it. The only reason for that is to save typing 5 characters. There
> is no other benefit that I can see. You don't save anyone *thinking* about
> const, because they still have to know whether this string is modifiable
> or needs freeing. You just remove the expression of that thought in the
> code, and consequently remove the compiler's ability to check whether they
> really thought about it, and encourage people to forget to think about it. 
> 
> There *are* cases where this will inconvenience people doing things the
> right way. For one, the compiler won't catch write-to-const-string
> screwups (and this is a gtk-list FAQ, and an annoying bug to find I'm sure
> we have all encountered at some point). For another, overloaded functions
> in C++ will not work properly.

How so? - overloading between const char * and char * is a really
frightening thought...

>  Gtk+ will not compile properly with an ANSI
> C++ compiler, because the type of a string literal is static const char*.

That is not an issue - there are many other reasons why GTK+
itself can't be compiled with a compiler in C++ mode - and no reason
why it should.

> And the code will be significantly less self-documenting.

Well, that could be solved with a few comments.

> Those things are not a huge deal, but they are IMO a much bigger deal than
> the desire to avoid warnings for things that do indeed merit a warning. 

Things like: 

 char *somevar = gtk_entry_get_text ();
 g_print (somevar);

? ;-)  95% of such cases do not merit a warning at all.

> Esp. when you can avoid the warning by simply typing "const" or
> "(gchar*)".
> 
> So that's my rant, and I hope I have convinced you. If not I guess I can
> "fix" the patch, but I think it's the wrong thing to do.

I'm not fanatical about the issue - const can be a useful tool, or so
I hear, (everybody has their own set of mistakes they make - writing
to const strings is not one of mine)

For passing function pointers in GLIB, where total compatiblity with
both the const-correct crowd and other people was impossible, I've
previously went with the 'const'.

But, my inclination is still that constant returns are more trouble
than they are worth - some support for this view can be found in the
standard C library, which is const-correct on the arguments, but not
on the returns.

Regards,
                                        Owen