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?