Re: [2/6] msxml3: IXMLDOMSchemaCollection/XMLSchemaCache backend

[Nikolay Sivov]

On 10/15/2010 18:25, Adam Martinson wrote:
> Implemented on top of libxml's hash table.
> ---
>  dlls/msxml3/schema.c |  376 
> +++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 326 insertions(+), 50 deletions(-)
> +static LONG cache_entry_release(cache_entry* entry)
>   {
> -    schema_t *This = impl_from_IXMLDOMSchemaCollection( iface );
> +    LONG ref = InterlockedDecrement(&entry->ref);
> +    TRACE("%p new ref %d\n", entry, ref);
>
> -    TRACE("(%p)->(%s %p)\n", This, debugstr_guid( riid ), ppvObject );
> +    if (ref == 0)
> +    {
> +        if (entry->type == SCHEMA_TYPE_XSD)
> +        {
> +            xmldoc_release(entry->doc);
> +            entry->schema->doc = NULL;
> +            xmlSchemaFree(entry->schema);
> +            heap_free(entry);
> +        }
> +        else /* SCHEMA_TYPE_XDR */
> +        {
> +            xmldoc_release(entry->doc);
> +            heap_free(entry);
> +        }
> +    }
> +    return ref;
> +}

Looks a but redundant. xmldoc_release() and heap_free(entry) are always 
used, so only xmlSchemaFree() and doc reset before it should be under 
condition.
> +    xmlNodePtr root;
> +    if (schema)
> +        root = xmlDocGetRootElement(schema);
> +    if (root&&  root->ns)
> +    {
> root uninitialized? If this never happens practically then remove if 
> (schema) condition.
> -    IXMLDOMSchemaCollection_AddRef( iface );
> +    IXMLDOMSchemaCollection_AddRef(iface);
>
> return S_OK;
>   }
>
> -static ULONG WINAPI schema_cache_AddRef( IXMLDOMSchemaCollection *iface )
> +static ULONG WINAPI schema_cache_AddRef(IXMLDOMSchemaCollection *iface)

No need for such changes here and later in this patch, even some new 
added lines use white-spaces in such cases.
> +                IXMLDOMNode* domnode = NULL;
> +                IDispatch_QueryInterface(V_DISPATCH(&var),&IID_IXMLDOMNode, (void**)&domnode);
> +
> +                if (domnode)
> +                    doc = xmlNodePtr_from_domnode(domnode, XML_DOCUMENT_NODE)->doc;
> Not sure about that, I'd say it's better to test for hr == S_OK and 
> return value from QueryInterface, is this branch tested?