Re: [evolution-patches] [calendar-plugins]: fix for bug #305627

["Carsten Guenther" <Carsten Guenther scalix com>]

I strongly feel that we should roll-back the original patch for the  
following reasons:

1. questionable code quality (see Christian's list of issues)
2. this is a "fix" for a non-critical enhancement request during  
feature-freeze
3. this patch not only introduces something Groupwise-specific (which  
is a bad thing by itself) but also introduces a performance penalty  
for all the other providers

Carsten

On Aug 17, 2005, at 8:18 PM, Christian Kellner wrote:

> I was just looking through e-p patches and saw this patch and spotted
> these things:
>
> 1)
>
> +        url = parent_object.url;
> +        uri = camel_url_to_string (url, CAMEL_URL_HIDE_ALL);
> +
> +        groups = e_source_list_peek_groups (pitip-> ...
>
> camel_url_to_string () gives you a newly malloc'ed string you should
> free, which you don't.
>
> 2)
>
> +         if (!strcmp (uri, e_source_get_uri (source))) {
> +                    found = TRUE;
> +                    break;
> +         }
>
> e_source_get_uri () also gives you a newly malloc'ed uri which you
> should free. Doing that in a loop is even worse.
>
> Some people out there are really trying hard to to spot old leaks  
> so we
> should be very careful when writing new code, especially if we are so
> close to a release.
>
> 3)
> +                CamelService parent_object;
> +                CamelURL *url;
> [...]
> +                parent_store = folder->parent_store;
> +                parent_object = parent_store->parent_object;
> +                url = parent_object.url;
>
> Really complicated code for for a simple dereference.
>
>
> Attached is a patch to fix these issues.
>
> Yours,
> Christian
>
> P.S.: On a side note, that doesn't really look like a bug fix for me,
> but more as a speed enhancement for GW although we were in feature
> freeze. Oh well.
>
> <itip.patch>
> _______________________________________________
> evolution-patches mailing list
> evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches