[PATCH v2 2/5] d2d1: Partially implement RegisterEffectFromStream().

[Ziqing Hui]

On 6/12/22 4:30 PM, Nikolay Sivov wrote:
> 
> 
> On 6/12/22 08:31, Ziqing Hui wrote:
>> +    /* Loop inside effect node */
>> +    end_node_found = FALSE;
>> +    while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK)
>> +    {
>> +        if (node_type == XmlNodeType_Element)
>> +        {
>> +            if (!wcscmp(node_name, L"Property"))
>> +                hr = parse_property(xml_reader, reg);
>> +            else if (!wcscmp(node_name, L"Inputs"))
>> +                hr = parse_inputs(xml_reader, reg);
>> +            else
>> +                hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
>> +
>> +            if (FAILED(hr))
>> +                goto done;
>> +        }
>> +        else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect"))
>> +        {
>> +            end_node_found = TRUE;
>> +            break;
>> +        }
>> +    }
>> +    hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
> I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
> 
> Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
> 

I'm thinking of failure case. The tests in the PATCH 1 show that if we meet element that are not expected, the function will return a error code (mostly HRESULT_FROM_WIN32(ERROR_NOT_FOUND)).
So I think we should not skip elements we don't interested in, we should return error for them.

Checking EndElement name is for the same reason.

Nikolay, what do you think?