Re: [PATCH 3/3] shell32: Move the pidl handling to the ExtractIcon_Constructor

[Nikolay Sivov]

On 5/20/2012 18:47, Detlef Riekenberg wrote:
> } else if (IsEqualIID(riid,&IID_IExtractIconA)&&(cidl == 1)) {
> -	    pidl = ILCombine(This->pidlRoot, apidl[0]);
> -	    pObj = (LPUNKNOWN) IExtractIconA_Constructor(pidl);
> -	    SHFree(pidl);
> -	    hr = S_OK;
> +            return IExtractIconAW_Constructor(This->pidlRoot, apidl[0], FALSE, ppvOut);
>   } else if (IsEqualIID(riid,&IID_IExtractIconW)&&(cidl == 1)) {
> -	    pidl = ILCombine(This->pidlRoot, apidl[0]);
> -	    pObj = (LPUNKNOWN) IExtractIconW_Constructor(pidl);
> -	    SHFree(pidl);
> -	    hr = S_OK;
> +            return IExtractIconAW_Constructor(This->pidlRoot, apidl[0], TRUE, ppvOut);
>   } else if ((IsEqualIID(riid,&IID_IShellLinkW) || IsEqualIID(riid,&IID_IShellLinkA))
> I think it's better to pass riid to constructor helper and return 
> appropriate vtable according to that riid. That will also collapse this 
> to a single 'if' testing for both interfaces.
> +HRESULT IExtractIconAW_Constructor(LPCITEMIDLIST root, LPCITEMIDLIST item, BOOL is_unicode, void ** ppvOut)
>   {
> -    IExtractIconWImpl *ei;
> +    IExtractIconWImpl *ei = HeapAlloc(GetProcessHeap(), 0, sizeof(IExtractIconWImpl));
> +    LPITEMIDLIST pidl = ILCombine(root, item);
>
> -    TRACE("%p\n", pidl);
> +    if (!pidl || !ei) {
> +        SHFree(pidl);
> +        HeapFree(GetProcessHeap(), 0, ei);
> +        return E_OUTOFMEMORY;
> +    }
>
> -    ei = HeapAlloc(GetProcessHeap(), 0, sizeof(*ei));
>       ei->ref=1;
>       ei->IExtractIconW_iface.lpVtbl =&eivt;
>       ei->IExtractIconA_iface.lpVtbl =&eiavt;
>       ei->IPersistFile_iface.lpVtbl =&pfvt;
>       ei->pidl=ILClone(pidl);
>
> -    pdump(pidl);
> Same here as in previous patch - storing combined result vs. making 
> another clone.