Re: [PATCH] : Final clean-up patch for address-entry

[Emmanuel <e allaud wanadoo fr>]

On 11.02.2002 20:51 Peter Bloomfield wrote:
> On 2002.02.11 08:43 Emmanuel wrote:
>> 	Hi all,
>> just a janitorial work on address-entry to have an almost clean source 
>> + some improvment for libbalsa_strsplit (simpler, faster) and 
>> libbalsa_fill_input.
>> This is the last patch on address-entry, on my part.
>> On current CVS
>> Bye
>> Manu
> 
> A few thoughts:
> 
> Why change the declaration of libbalsa_strsplit:
>> -static GList *
>> +GList *
>>  libbalsa_strsplit(const gchar *str, gchar delimiter)

That's a bug (I probably messed up different versions).

> Here you could trim leading blanks, if it helps later:
>> +    for (current=str;*current;current++) {
>> +	if (*current == '"') quoted = !quoted;
>> +	else if ( (!quoted) && (*current == delimiter) ) {
>> +	    glist = g_list_append(glist, g_strndup(old,
>> current-old));
>             while (current[1] == ' ')
>                 ++current;
> +           old=current+1;
>>  	}

Yes, I'll changed that.

> Don't you still need a test here? What if str ended with a delimiter?
>> -    if (str) {
>> -	data = g_strndup(old, i - previous);
>> -	glist = g_list_append(glist, data);
>>      }
>> +    glist = g_list_append(glist, g_strndup(old, current-old));

Humm I just removed it because the whole code is yet protected by a if 
(str) return; and in the original code you'll see that str does not 
change, so this is useless. But I didn't check if the original code was 
not buggy ;-) But I don't think it was. I have tested a lot of corner 
cases, but anyway I'll look further.

> The first time through this loop, addy points to the current address--is 
> that what you want?
>      size = prev = 0;
> -    for (list = g_list_first(address_entry->active);
> -        list != NULL;
> -        list = g_list_next(list)) {
> -       if (cursor >= size) {
> -           prev = size;
> -           address_entry->active = list;
> -       }
> +    for (list = address_entry->active;list;list = g_list_next(list)) {
> +       prev = size;
> +       size += strlen(addy->user) + 1;
>         addy = (emailData *)list->data;

No, in this code we are constructing the list and address_entry->active is 
actually pointing to the beginning of the list, so no need to call 
g_list_first. But I agree that this does not respect the semantic of 
addres_entry->active. Perhaps we should use another variable in the 
function.
Moreover Peter pointed me to one or two bugs and he sent me an enhanced 
patch. I'll do some tests and send a reworked patch to the list. So please 
wait and don't apply this one (Ah! At least next time will be OK for 
merging, damn!).
Bye
Manu