Re: [g-a-devel]gnopernicus cleanup patch ...

[Michael Meeks <michael ximian com>]

Hi Adi,

On Mon, 2003-06-30 at 16:03, Adi Dascal wrote:
> Thanks a lot for this patch and sorry for answering so late, but we all were 
> in vacation until today.

	No problem; it was lovely to meet you (all) - I hope you had a good
break.

> Please go ahead and commit !

	Done - needless to say there is prolly a lot more scope for this sort
of cleanup eg. in the same module this fragment:

    if (!style)
    	style = g_strdup ("default");
	
    message = src_xml_make_part ("TEXT", src_speech_get_voice
("system"), style);
    src_speech_send_chunk (message, SRC_SPEECH_PRIORITY_IDLE);
    g_free (message);
    g_free (style);

	seems to have been cut and pasted several times; similarly some of this
sort of flow:

    if (sro_text_get_attributes_at_index (src_crt_sro, src_text_index,
&attr, SR_INDEX_CONTAINER))
    {
	gchar *tmp;
	if (sra_get_attribute_value (attr[0], "style", &tmp))
	{
	    style = g_strdup (tmp);
	    SR_freeString (tmp);
	}
	sra_free (attr);
    }
    
	etc. could be crunched down to a couple of lines with something like:

gchar *text = NULL;
if (foo_get_attr (ctxt, &attr))
     str_get_attr_value (attr[0], "style", &text);

do_say_with_default (ctxt, text);

SR_freeString (text);

	Saving a lot of lines, a good number of dups, etc. IMHO
fewer lines == greater value :-) almost always. I'm almost tempted to make a "cut
and paste code detection" script to run over code :-)
 
> Please see my comments/questions regarding compile-time warnings in the body 
> of the original mail, between <ADI> tags. 
...
> We are aware that there is global-variablizing in our code and that we MUST 
> fix it, but compile-time errors we can NOT see.

	Ah - ok.

> We are not using emacs, but compiling with make from command line. In order 
> to detect unwanted warnings we set CFLAGS='-Wall -g -Werror',

	Right - that's good. Of course, you need to set CFLAGS before you
configure otherwise they are not used AFAIR. Looking at the errors
again, I think it's likely that I'm using -O2 - which does a lot more
(slow) flow analysis on the code than the default -O0 - and thus can
generate more warnings.

eg.

srspc.c: In function `src_speech_process_string':
srspc.c:1472: warning: `tmp' might be used uninitialized in this
function
srspc.c: In function `src_speech_modify_pitch':
srspc.c:1595: warning: `pitch' might be used uninitialized in this
function
srspc.c: In function `src_speech_modify_rate':
srspc.c:1645: warning: `rate' might be used uninitialized in this
function
srspc.c: In function `src_speech_modify_volume':
srspc.c:1696: warning: `volume' might be used uninitialized in this
function

	This warning is sometimes wrong; but often right. Worse - the last 3
warnings reveal another triple chunk of cut and paste coding that could
do with fixing ;->

	Of course - the problem with using -O2 is that it fairly dramatically
reduces debuggability in some strange ways; but having a healthy
scepticism of what the debugger shows you is mostly a good thing I
guess.

	Perhaps it's worth using -O2 occasionally just to see.

	Hope that helps,

	Regards,

		Michael.

-- 
 michael ximian com  <><, Pseudo Engineer, itinerant idiot