Re: [PATCH 1/3] d3dx9_36: Add support for DIB file in D3DXGetImageInfoFromFileInMemory. (try 2)

[Rico Schüller]

On 22.04.2013 23:08, Christian Costa wrote:
> +typedef struct {
> - DWORD bc2Size;
> - DWORD bc2Width;
> - DWORD bc2Height;
> - WORD  bc2Planes;
> - WORD  bc2BitCount;
> - DWORD bc2Compression;
> - DWORD bc2SizeImage;
> - DWORD bc2XRes;
> - DWORD bc2YRes;
> - DWORD bc2ClrUsed;
> - DWORD bc2ClrImportant;
> - /* same as BITMAPINFOHEADER until this point */
> - WORD  bc2ResUnit;
> - WORD  bc2Reserved;
> - WORD  bc2Orientation;
> - WORD  bc2Halftoning;
> - DWORD bc2HalftoneSize1;
> - DWORD bc2HalftoneSize2;
> - DWORD bc2ColorSpace;
> - DWORD bc2AppData;
>
> +} BITMAPCOREHEADER2;

Do we really need a typedef here? For only locally used structs, I think 
we shouldn't use a typedef. Also the struct is only used to determine 
the size and for nothing else? There may be a better way to do that... 
Maybe something like in dlls/windowscodecs/icoformat.c ?

I think you got inspired by dlls/windowscodecs/bmpdecode.c, there is the 
same issue. Why do we need a typedef, if we only use it in one file to 
calculate the size?
> -
>
> BITMAPFILEHEADER *header;
> -
>
> if ((header_size == sizeof(BITMAPINFOHEADER)) ||
> -
>    (header_size == sizeof(BITMAPV4HEADER)) ||
> -
>    (header_size == sizeof(BITMAPV5HEADER)) ||
> -
>    (header_size == sizeof(BITMAPCOREHEADER2)))
> -
>
> {
> -
>    /* All structures have the same memory layout as BITMAPINFOHEADER */
> -
>    BITMAPINFOHEADER*header = (BITMAPINFOHEADER*)*data;
> -
>    ULONG count = header->biClrUsed;
>
> -
>
> else if (header_size == sizeof(BITMAPCOREHEADER))
> -
>
> {
> -
>    BITMAPCOREHEADER*header = (BITMAPCOREHEADER*)*data;

I don't think it's a good idea to use variable names multiple times. 
While this works fine it may be a bit confusing...

Cheers
Rico

[PATCH 1/3] d3dx9_36: Add support for DIB file in D3DXGetImageInfoFromFileInMemory. (try 2)

[Christian Costa]

2013/4/23 Rico Schüller
> On 22.04.2013 23:08, Christian Costa wrote:
> > +typedef struct {
> > - DWORD bc2Size;
> > - DWORD bc2Width;
> > - DWORD bc2Height;
> > - WORD  bc2Planes;
> > - WORD  bc2BitCount;
> > - DWORD bc2Compression;
> > - DWORD bc2SizeImage;
> > - DWORD bc2XRes;
> > - DWORD bc2YRes;
> > - DWORD bc2ClrUsed;
> > - DWORD bc2ClrImportant;
> > - /* same as BITMAPINFOHEADER until this point */
> > - WORD  bc2ResUnit;
> > - WORD  bc2Reserved;
> > - WORD  bc2Orientation;
> > - WORD  bc2Halftoning;
> > - DWORD bc2HalftoneSize1;
> > - DWORD bc2HalftoneSize2;
> > - DWORD bc2ColorSpace;
> > - DWORD bc2AppData;
> >
> > +} BITMAPCOREHEADER2;
>
> Do we really need a typedef here? For only locally used structs, I think
> we shouldn't use a typedef. Also the struct is only used to determine the
> size and for nothing else? There may be a better way to do that... Maybe
> something like in dlls/windowscodecs/icoformat.c ?
>
> I think you got inspired by dlls/windowscodecs/bmpdecode.**c, there is
> the same issue. Why do we need a typedef, if we only use it in one file to
> calculate the size?

This is correct. Indeed using header_size == 64 /*
sizeof(BITMAPCOREHEADER2) */ would be better.
> > -
> >
> > BITMAPFILEHEADER *header;
> > -
> >
> > if ((header_size == sizeof(BITMAPINFOHEADER)) ||
> > -
> >    (header_size == sizeof(BITMAPV4HEADER)) ||
> > -
> >    (header_size == sizeof(BITMAPV5HEADER)) ||
> > -
> >    (header_size == sizeof(BITMAPCOREHEADER2)))
> > -
> >
> > {
> > -
> >    /* All structures have the same memory layout as
> >
> > BITMAPINFOHEADER */
> > -
> >    BITMAPINFOHEADER*header = (BITMAPINFOHEADER*)*data;
> >
> > -
> >    ULONG count = header->biClrUsed;
> >
> > -
> >
> > else if (header_size == sizeof(BITMAPCOREHEADER))
> > -
> >
> > {
> > -
> >    BITMAPCOREHEADER*header = (BITMAPCOREHEADER*)*data;
>
> I don't think it's a good idea to use variable names multiple times. While
> this works fine it may be a bit confusing...

I don't find it confusing but I don't mind changing it.