Re: [PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

[Matteo Bruni]

2016-09-13 22:11 GMT+02:00 Fabian Maurer :
> Signed-off-by: Fabian Maurer
>
> dlls/d3dx11_43/async.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/d3dx11_43/async.c b/dlls/d3dx11_43/async.c
> index 90c60fa..1e3042b 100644
> --- a/dlls/d3dx11_43/async.c
> +++ b/dlls/d3dx11_43/async.c
> @@ -1989,16 +1989,36 @@ HRESULT WINAPI D3DX11CompileFromFileA(const char *filename, const D3D10_SHADER_M
>      return E_NOTIMPL;
>  }
> -

Stray newline.
> HRESULT WINAPI D3DX11CompileFromFileW(const WCHAR *filename, const D3D10_SHADER_MACRO *defines,
>          ID3D10Include *include, const char *entry_point, const char *target, UINT sflags, UINT eflags,
>          ID3DX11ThreadPump *pump, ID3D10Blob **shader, ID3D10Blob **error_messages, HRESULT *hresult)
>  {
> - FIXME("filename %s, defines %p, include %p, entry_point %s, target %s, sflags %#x, "
> - void *file_buffer;
> - HRESULT hr;
> - DWORD file_size;
> - LPSTR filename_converted;

Please use "char *" instead.
> - UINT filename_converted_len;

Probably you can call it just "size".
> -
> - TRACE("filename %s, defines %p, include %p, entry_point %s, target %s, sflags %#x, "
>         "eflags %#x, pump %p, shader %p, error_messages %p, hresult %p stub.\n",

The "stub" part should go away.
>          debugstr_w(filename), defines, include, debugstr_a(entry_point), debugstr_a(target),
>          sflags, eflags, pump, shader, error_messages, hresult);
>
> - return E_NOTIMPL;
> - if (FAILED(hr = map_view_of_file(filename, &file_buffer, &file_size)))
> -
>    return hr;
> -
> - filename_converted_len = WideCharToMultiByte(CP_ACP, 0, filename, -1, NULL, 0, NULL, NULL);
> - filename_converted = HeapAlloc(GetProcessHeap(), 0, filename_converted_len);
> - WideCharToMultiByte(CP_ACP, 0, filename, -1, filename_converted, filename_converted_len, NULL, NULL);
> -
> - hr = D3DX11CompileFromMemory(file_buffer, file_size, filename_converted, defines, include, entry_point, target,
> -
>        sflags, eflags, pump, shader, error_messages, hresult);
> -
> - HeapFree(GetProcessHeap(), 0, filename_converted);
> - UnmapViewOfFile(file_buffer);
> -
> - return hr;
>
> }

This would be okay otherwise but, as mentioned for patch [4/5], maybe
it's a good idea to implement and make use of
D3DX11CreateAsyncFileLoaderW() here too.

[PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

[Fabian Maurer]

Thanks for your feedback.
> > - UINT filename_converted_len;
>
> Probably you can call it just "size".

Are you sure? I tried to make it less ambiguous, since there are two sizes 
used.
> This would be okay otherwise but, as mentioned for patch [4/5], maybe
> it's a good idea to implement and make use of
> D3DX11CreateAsyncFileLoaderW() here too.

I fixed the other issues on my local branch and I'll try to get the async parts 
right.

[PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

[Matteo Bruni]

2016-09-20 1:56 GMT+02:00 Fabian Maurer :
> Thanks for your feedback.
> > > - UINT filename_converted_len;
> >
> > Probably you can call it just "size".
>
> Are you sure? I tried to make it less ambiguous, since there are two sizes
> used.

Right. Whatever is fine I guess, but using "_size" instead of "_len"
seems valuable for consistency.
> > This would be okay otherwise but, as mentioned for patch [4/5], maybe
> > it's a good idea to implement and make use of
> > D3DX11CreateAsyncFileLoaderW() here too.
>
> I fixed the other issues on my local branch and I'll try to get the async parts
> right.

Cool, looking forward to it.