Re: [PATCH 05/10] d3d11: Implement D3D11CreateDeviceAndSwapChain().

[Henri Verbeet]

On 14 September 2015 at 00:13, Józef Kucia  wrote:
> - if (swapchain_out)
> -
>    *swapchain_out = swapchain;
> - else
> -
>    IDXGISwapChain_Release(swapchain);

Is "desc" validated or modified in this case? Otherwise you could
return a lot earlier. The test doesn't really cover that.
> - hr = D3D11CreateDevice(adapter, driver_type, swrast, flags, feature_levels, levels, sdk_version,
> -
>        &device, NULL, NULL);
> - if (FAILED(hr))
> - {
> -
>    WARN("Failed to create a device, returning %#x.\n", hr);
> -
>    *device_out = NULL;
> -
>    return hr;
> - }

...
> -
> - if (obtained_feature_level)
> -
>    *obtained_feature_level = ID3D11Device_GetFeatureLevel(device);
> -
> - if (immediate_context)
> -
>    ID3D11Device_GetImmediateContext(device, immediate_context);

This seems odd. Why not just pass these to D3D11CreateDevice()?

If you like you can do things like the D3D11CreateDevice() call inside
the if. I.e., "if (FAILED(hr = D3D11CreateDevice(...)))". I tend to do
that for newer code, older code usually has it on separate lines.

[PATCH 05/10] d3d11: Implement D3D11CreateDeviceAndSwapChain().

[Józef Kucia]

On Mon, Sep 14, 2015 at 1:48 PM, Henri Verbeet  wrote:
> On 14 September 2015 at 00:13, Józef Kucia  wrote:
> > - if (swapchain_out)
> > -
> >    *swapchain_out = swapchain;
> > - else
> > -
> >    IDXGISwapChain_Release(swapchain);
>
> Is "desc" validated or modified in this case? Otherwise you could
> return a lot earlier. The test doesn't really cover that.

You're right. The test should cover this case.
> > - hr = D3D11CreateDevice(adapter, driver_type, swrast, flags, feature_levels, levels, sdk_version,
> > -
> >        &device, NULL, NULL);
> > - if (FAILED(hr))
> > - {
> > -
> >    WARN("Failed to create a device, returning %#x.\n", hr);
> > -
> >    *device_out = NULL;
> > -
> >    return hr;
> > - }
>
> ...
> > -
> > - if (obtained_feature_level)
> > -
> >    *obtained_feature_level = ID3D11Device_GetFeatureLevel(device);
> > -
> > - if (immediate_context)
> > -
> >    ID3D11Device_GetImmediateContext(device, immediate_context);
>
> This seems odd. Why not just pass these to D3D11CreateDevice()?

The obtained_feature_level should definitely be passed to
D3D11CreateDevice. When it comes to the immediate context, the intent
was to get it only when the function succeeds so it doesn't have to be
released in every case in which the function fails.

[PATCH 05/10] d3d11: Implement D3D11CreateDeviceAndSwapChain().

[Henri Verbeet]

On 14 September 2015 at 16:29, Józef Kucia  wrote:
> The obtained_feature_level should definitely be passed to
> D3D11CreateDevice. When it comes to the immediate context, the intent
> was to get it only when the function succeeds so it doesn't have to be
> released in every case in which the function fails.

It shouldn't be too bad if you jump to the cleanup code with goto, I think.