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.