Re: const fixes seek commit approval




On 26 Oct 1998, Owen Taylor wrote:
> 
> By 'no breakage' - are you saying that there are no
> additional warnings at all, or just that it did compile.
>

I changed the const stuff, then made clean and rebuilt over and over with
-Wall -ansi -pedantic, fixing stuff until all warnings that could
conceivably be my fault went away. (there are still a bunch of other
warnings, but I don't think I caused them :-) 

As I said there are still two warnings in gtkfontsel.c. I think I fixed
everything "right", i.e. without using casts, and I was unsure what
"right" would be in those two cases.
 
> 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.
>  

The patch is largish, true.The changes are trivial though, took a couple
hours and I doubt anything could be broken. The change is almost always
like this: 

- char* ltext, rtext;
+ char* rtext;
+ const char* ltext;

The other change is adding const to a function arg, then to the args of
the functions it calls, etc., etc. until you run out of functions.

> 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.
>
  
This is true for C. If it's const, you know it *isn't* modifiable, but if
it's not const things are indeterminate. For C++ this is theoretically not
the case, except when interfacing with C libraries. ;-)

> > 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 see your point; this is even the motivation for the patch. Gtk+ is the
last non-correct interface being used by my program. However it's true
that I don't think return values are causing any of the warnings in my
code.

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

C lets you do this implicitly... at least I get no warning.
 
> > 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.
> 

So put a const in front of the char*, problem solved. ;-)

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

OK, you have convinced me to be at least kind of neutral on the issue, and
I want to go ahead and get the const args stuff in. So I will commit a
const args patch tomorrow or so when I get back to a computer I can
compile stuff on, if there are no objections by then. 

One remaining issue, what do you think of this kind of return value:

gtk_label_get(GtkLabel*, char** text);

I changed this to const char**, as you might predict. Is this a return
value or an arg for the purposes of redoing the patch? It looks to me like
it has the disadvantages (and advantages) of each, sadly. Sigh. 

Havoc




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