Re: [PATCH 1/1] d3d11: Validate layout description in d3d_input_layout_create().

[Zebediah Figura (she/her)]

On 8/9/22 13:13, Paul Gofman wrote:
> +static BOOL is_mandatory_shader_input(const struct wined3d_shader_signature_element *e)

Nitpick, but this feels like a slightly awkward name—it doesn't really 
tell you what it's mandatory for (or what kind of input it's measuring).

I kind of want to invert it and call it "is_sysval_semantic", since 
that's the idea, even though in practice it's not using the criteria you 
would expect.
> -
>    for (j = 0; j < element_count; ++j)
> -
>    {
> -
>        f = &element_descs[j];
> -
>        if (!stricmp(ise[i].semantic_name, f->SemanticName) && ise[i].semantic_idx == f->SemanticIndex
> -
>                && !ise[i].stream_idx)
> -
>        {
> -
>            (*wined3d_elements)[j].output_slot = ise[i].register_idx;
> -
>            break;
> -
>        }
> -
>    }
> -
>    if (j == element_count && is_mandatory_shader_input(&ise[i]))
> -
>    {
> -
>        WARN("Input element %s%u not found in shader signature.\n", ise[i].semantic_name, ise[i].semantic_idx);
> -
>        free(*wined3d_elements);
> -
>        free(is.elements);
> -
>        return E_INVALIDARG;
> -
>    }

A matter of personal style, but I prefer to use a helper function rather 
than this for-if pattern.
> - D3D11_INPUT_ELEMENT_DESC elemets_temp[32];

Typo, "elemets".
> - ID3D11InputLayout *layout;
> - unsigned int i;
> - HRESULT hr;
> -
> - winetest_push_context(test_context);
> -
> - hr = ID3D11Device_CreateInputLayout(device, elements, element_count, shader_code, shader_code_size, &layout);
> - ok(hr == S_OK, "Got hr %#lx.\n", hr);
> - ID3D11InputLayout_Release(layout);
> -
> - for (i = 0; i < element_count; ++i)
> - {
> -
>    winetest_push_context("%s excluded", elements[i].SemanticName);
> -
>    memcpy(elemets_temp, elements, sizeof(*elemets_temp) * i);
> -
>    memcpy(elemets_temp + i, elements + i + 1, sizeof(*elemets_temp) * (element_count - i - 1));
> -
>    hr = ID3D11Device_CreateInputLayout(device, elemets_temp, element_count - 1,
> -
>            shader_code, shader_code_size, &layout);
> -
>    if (sysval_index == ~0u || i == sysval_index)
> -
>    {
> -
>        /* sysval semantic. */
> -
>        ok(hr == S_OK, "Got hr %#lx.\n", hr);
> -
>        ID3D11InputLayout_Release(layout);

This is a bit weird, because my natural reaction is, "well, of course 
you get S_OK; those are sysval semantics and you're not supposed to find 
them in the input layout". If anything it's interesting to test that 
it's legal to specify them anyway.

That said, the "sysval_index" thing is slightly harder to read, perhaps 
you could just key this on the input element name and copy the d3d11 
function here?
> - layout_desc->Format = vertex_formats[0];

The existing code does this, but if it's an array with multiple elements 
could you please use "layout_desc[0]." rather than "layout_desc->"?