Re: [2/2]dwrite: basic implementation for FontCollectionLoader registration

[Piotr Caban]

On 8/1/14 12:01 PM, Aric Stewart wrote:
> - if (i == This->CollectionLoaderCount)
> - {
> -
>    This->CollectionLoaderCount++;
> -
>    if (This->CollectionLoaders)
> -
>        This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> -
>    else
> -
>        This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> -
>    if (!This->CollectionLoaders)
> -
>        return E_OUTOFMEMORY;
> - }

There's a leak in out of memory case. Isn't it better to grow the buffer 
exponentially?

[2/2]dwrite: basic implementation for FontCollectionLoader registration

[Aric Stewart]

On 8/1/14, 12:28 PM, Piotr Caban wrote:
> On 8/1/14 12:01 PM, Aric Stewart wrote:
> > - if (i == This->CollectionLoaderCount)
> > - {
> > -
> >    This->CollectionLoaderCount++;
> > -
> >    if (This->CollectionLoaders)
> > -
> >        This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > -
> >    else
> > -
> >        This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > -
> >    if (!This->CollectionLoaders)
> > -
> >        return E_OUTOFMEMORY;
> > - }
>
> There's a leak in out of memory case. Isn't it better to grow the buffer exponentially?

I do not think a given application will be registering very many of these. No experience yet but I would be surprised at more than 2. So I am not sure if exponential is the right course.  But it is an easy change.

Maybe i have been spending too much time out of C recently, but I am totally failing to see the leak. Other than the face that our loader object will be in a corrupt state, I can fix the corrupt state but I am not sure if that would be fixing the memory leak you see.

-aric

[2/2]dwrite: basic implementation for FontCollectionLoader registration

[Henri Verbeet]

On 4 August 2014 15:18, Aric Stewart  wrote:
> I do not think a given application will be registering very many of these. No experience yet but I would be surprised at more than 2. So I am not sure if exponential is the right course.  But it is an easy change.

There are only very few cases where doubling the array size isn't the
right thing to do.
> Maybe i have been spending too much time out of C recently, but I am totally failing to see the leak.

If heap_realloc() fails, the previous allocation is lost because you
end up assigning NULL to This->CollectionLoaders. I personally also
would prefer to write the sizeof's as
"sizeof(*This->CollectionLoaders)".
> - if (i == This->CollectionLoaderCount)
> -
>        return E_INVALIDARG;

Formatting.