Re: const fixes seek commit approval




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. 

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.

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. 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.

Note that you are not avoiding the cast by not using const; you are just
moving it into the library and making it implicit, 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. Gtk+ will not compile properly with an ANSI
C++ compiler, because the type of a string literal is static const char*.
And the code will be significantly less self-documenting.

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. 
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.

Thanks,
Havoc














[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]