Re: [PATCH 3/6] gameux: add support of loading Application Id into registry

[Nikolay Sivov]

On 9/3/2010 13:25, Mariusz Pluciński wrote:
> +static HRESULT _ParseGameDefinition(
> +        IXMLDOMElement *lpXMLGameDefinitionElement,
> +        struct GAME_DATA *GameData)
> +{
> +    static const WCHAR sGameId[] = {'g','a','m','e','I','D',0};
> +
> +    HRESULT hr = S_OK;
> +    BSTR bstrAttribute = NULL;
> +    VARIANT variant;
> No need for initializers here.
> +    hr = IXMLDOMElement_getAttribute(lpXMLGameDefinitionElement, bstrAttribute,&variant);
> +    if(hr != S_OK || V_VT(&variant) != VT_BSTR) hr = E_FAIL;
> No need to test for hr != S_OK and alter failure code after that I guess.
> +    /* load MSXML parser */
> +    hr = CoCreateInstance(&CLSID_DOMDocument30, NULL, CLSCTX_INPROC_SERVER,
> +&IID_IXMLDOMDocument, (LPVOID*)&lpXMLDocument);
> Use neutral CLSID_DOMDocument. Minor things - useless comment, and 
> LPVOID* -> void**.
> +    IXMLDOMDocument *lpXMLDocument = NULL;
> +    IXMLDOMNode *lpXMLGameDefinitionNode = NULL;
> +    IXMLDOMElement *lpXMLRootElement = NULL, *lpXMLGameDefinitionElement = NULL;
> Same here, you don't need to initialize that. Please use shorter names, 
> like 'root', 'document' or something.
> +            hr = IXMLDOMDocument_get_documentElement(lpXMLDocument,&lpXMLRootElement);
> +            if(hr == S_FALSE)
> +                hr = E_FAIL;
> hr != S_OK works.

P.S. it makes sense to test xml parsing part with native msxml first. 
This will hopefully show some problems with builtin msxml.

[PATCH 3/6] gameux: add support of loading Application Id into registry

[Mariusz Pluciński]

W dniu 03.09.2010 13:13, Nikolay Sivov pisze:
> On 9/3/2010 13:25, Mariusz Pluciński wrote:
> > + hr =
> > IXMLDOMDocument_get_documentElement(lpXMLDocument,&lpXMLRootElement);
> > + if(hr == S_FALSE)
> > + hr = E_FAIL;
> > hr != S_OK works.
> > I want this function to return E_FAIL in the case there's no root 
> > element, and other code error when get_documentElement encouraged other 
> > problem. If I do
> > if(hr != S_OK)
> >    hr = E_FAIL;
> > It will also replace other E_* codes with E_FAIL, and this will make 
> > problems discovering more complicated.
> P.S. it makes sense to test xml parsing part with native msxml first.
> This will hopefully show some problems with builtin msxml.
> OK, I'll this about this.