Re: d3dx9: Implementation of D3DXGetShaderSize

[H. Verbeet]

On 06/04/2008, Luis Busquets  wrote:
> This function obtains the size of a shader bytestream. It does so looking
> for the End token.
> +UINT D3DXGetShaderSize(DWORD * pFunction)

I would expect this to take a const pointer. Also, there's no reason
to use Microsoft's (broken) naming conventions for function arguments.
> - while (*address!=0x0000ffff) {
> - /*Let's check if the next DWORD is a comment*/
> - if ((*address&0x0000ffff)==0x0000fffe) {
> -
>            address=address+((*address >> 0x10) & 0x00007fff);
> -
>            }
> - address=address+0x1;
> - }

It would help readability quite a bit to use the proper constants
here. Also, please try to use an indentation style consistent with the
rest of the dll and wine (this goes for the other patches as well).
> - size=(address-pFunction+0x1)*sizeof(DWORD);

Why the +1 here? Does it return the size without the end token itself?
Tests and comments are always helpful there.

The following (untested!) implementation looks more reasonable to me:

UINT D3DXGetShaderSize(const DWORD *byte_code)
{
    const DWORD *ptr = byte_code;

if (!ptr) return 0;

/* Look for the END token, skipping the VERSION token */
    while (*++ptr != D3DSIO_END) {
        /* Skip comments */
        if ((*ptr & D3DSI_OPCODE_MASK) == D3DSIO_COMMENT) {
            ptr += ((*ptr & D3DSI_COMMENTSIZE_MASK) >> D3DSI_COMMENTSIZE_SHIFT);
        }
    }

/* Return the shader size in bytes, excluding the END token. */
    return (ptr - (byte_code + 1)) * sizeof(*ptr);
}

d3dx9: Implementation of D3DXGetShaderSize 2/3

[Luis Busquets]

The +1 of the end is because finally address points to the end token. So 
+1 is the end token. This function has been tested with all shaders of 
Civilization 4 and has responded successfully.

The attached patch has included the suggestions.

H. Verbeet escribió:
> On 06/04/2008, Luis Busquets  wrote:
> > This function obtains the size of a shader bytestream. It does so looking
> > for the End token.
> > +UINT D3DXGetShaderSize(DWORD * pFunction)
>
> I would expect this to take a const pointer. Also, there's no reason
> to use Microsoft's (broken) naming conventions for function arguments.
> > - while (*address!=0x0000ffff) {
> > - /*Let's check if the next DWORD is a comment*/
> > - if ((*address&0x0000ffff)==0x0000fffe) {
> > -
> >            address=address+((*address >> 0x10) & 0x00007fff);
> > -
> >            }
> > - address=address+0x1;
> > - }
>
> It would help readability quite a bit to use the proper constants
> here. Also, please try to use an indentation style consistent with the
> rest of the dll and wine (this goes for the other patches as well).
> > - size=(address-pFunction+0x1)*sizeof(DWORD);
>
> Why the +1 here? Does it return the size without the end token itself?
> Tests and comments are always helpful there.
>
> The following (untested!) implementation looks more reasonable to me:
>
> UINT D3DXGetShaderSize(const DWORD *byte_code)
> {
>     const DWORD *ptr = byte_code;
> if (!ptr) return 0;
>
> /* Look for the END token, skipping the VERSION token */
> while (*++ptr != D3DSIO_END) {
>     /* Skip comments */
>     if ((*ptr & D3DSI_OPCODE_MASK) == D3DSIO_COMMENT) {
>         ptr += ((*ptr & D3DSI_COMMENTSIZE_MASK) >> D3DSI_COMMENTSIZE_SHIFT);
>     }
> }
>
> /* Return the shader size in bytes, excluding the END token. */
> return (ptr - (byte_code + 1)) * sizeof(*ptr);
>
> }

d3dx9: Implementation of D3DXGetShaderSize 2/3

[H. Verbeet]

On 06/04/2008, Luis Busquets  wrote:
> The +1 of the end is because finally address points to the end token. So +1
> is the end token. This function has been tested with all shaders of
> Civilization 4 and has responded successfully.

My bad, I misread that as (address-(pFunction+0x1)) instead of
((address-pFunction)+0x1)
> - while (*address!=D3DVS_END()) {

I do think D3DSIO_END would be more appropriate here.