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

[Nikolay Sivov]

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.