Re: [1/2] windowscodecs: Add a test for CreateBitmapFromHICON.

[Nikolay Sivov]

On 1/11/2013 14:39, Dmitry Timoshkov wrote:
> - icon = CreateIconIndirect(&info);
> - ok(icon != 0, "CreateIconIndirect failed\n");
> - DeleteObject(info.hbmColor);
> - DeleteObject(info.hbmMask);
> -
> - hr = IWICImagingFactory_CreateBitmapFromHICON(factory, icon, &bitmap);
> - ok(hr == S_OK, "CreateBitmapFromHICON error %#x\n", hr);
> -
> - IWICBitmap_GetPixelFormat(bitmap, &format);
> - ok(IsEqualGUID(&format, &GUID_WICPixelFormat32bppBGRA),
> -
>   "unexpected pixel format %s\n", debugstr_guid(&format));
> -
> - IWICBitmap_GetSize(bitmap, &width, &height);
> - ok(hr == S_OK, "IWICBitmap_GetSize error %#x\n", hr);
> - ok(width == 16, "expected 16, got %u\n", width);
> - ok(height == 16, "expected 16, got %u\n", height);
> -
> - IWICBitmap_Release(bitmap);
>
> +}

Who is responsible for freeing icon handle? I guess it should be done 
here cause your implementation doesn't free it either.

[1/2] windowscodecs: Add a test for CreateBitmapFromHICON.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> Who is responsible for freeing icon handle? I guess it should be done 
> here cause your implementation doesn't free it either.

You are right, DestroyIcon() is missing at the end of the test. Consider
that as a feature, the tests are not supposed to free all the allocated
resources, it's a waste of time :)

[1/2] windowscodecs: Add a test for CreateBitmapFromHICON.

[Nikolay Sivov]

On 1/11/2013 14:58, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > Who is responsible for freeing icon handle? I guess it should be done
> > here cause your implementation doesn't free it either.
>
> You are right, DestroyIcon() is missing at the end of the test. Consider
> that as a feature, the tests are not supposed to free all the allocated
> resources, it's a waste of time :)

Fixing leaks like that is not optional. One reason to do this is to make 
valgrind output clean
so it contains only real leaks in code. Another reason is that test 
could serve as an example of API
usage.

[1/2] windowscodecs: Add a test for CreateBitmapFromHICON.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > Who is responsible for freeing icon handle? I guess it should be done
> > > here cause your implementation doesn't free it either.
> >
> > You are right, DestroyIcon() is missing at the end of the test. Consider
> > that as a feature, the tests are not supposed to free all the allocated
> > resources, it's a waste of time :)
>
> Fixing leaks like that is not optional. One reason to do this is to make 
> valgrind output clean
> so it contains only real leaks in code. Another reason is that test 
> could serve as an example of API
> usage.

I'd like to hear Alexandre's opinion on this, but freeing allocated memory
in tests has never been a requirement, and Wine tests are not supposed to be
a set of samples to learn windows programming.

[1/2] windowscodecs: Add a test for CreateBitmapFromHICON.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Nikolay Sivov  wrote:
> > > > Who is responsible for freeing icon handle? I guess it should be done
> > > > here cause your implementation doesn't free it either.
> > >
> > > You are right, DestroyIcon() is missing at the end of the test. Consider
> > > that as a feature, the tests are not supposed to free all the allocated
> > > resources, it's a waste of time :)
> >
> > Fixing leaks like that is not optional. One reason to do this is to make 
> > valgrind output clean
> > so it contains only real leaks in code. Another reason is that test 
> > could serve as an example of API
> > usage.
>
> I'd like to hear Alexandre's opinion on this, but freeing allocated memory
> in tests has never been a requirement, and Wine tests are not supposed to be
> a set of samples to learn windows programming.

In that kind of test it's a good idea to destroy the object right after
it's used, to catch cases where e.g. CreateBitmap would hold a reference
to the icon instead of copying it or things like that.