[PATCH 1/4] shell32: Add ShellLinkObject stubs.
Myah Caron
qsniyg at protonmail.com
Sun Sep 13 17:20:21 CDT 2020
Thanks for the review!
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, September 13, 2020 2:09 PM, Nikolay Sivov <nsivov at codeweavers.com> wrote:
> On 9/13/20 11:33 PM, Myah Caron wrote:
>
> > +typedef struct {
> >
> > - IShellLinkDual2 IShellLinkDual2_iface;
> > - LONG ref;
> > -
> > - FolderItemImpl *item;
> > - IShellLinkW *shell_link;
> > - IPersistFile *persist_file;
> > +} ShellLinkObjectImpl;
> >
>
> It looks like you're only using shell_link.
>
Sorry, I forgot to remove .item.
.persist_file is used for ::Release.
> > +static HRESULT ShellLinkObject_Constructor(FolderItemImpl *item, IShellLinkDual2 **link)
>
> It's not used until 3/4. I think it's fine to have tests in first patch,
> and merge this one with 3/4.
>
I don't mind merging them, but I was under the impression it would make the commit a little too large (~450 loc instead of ~300+~150) and unfocused (due to containing both stubs, and tests for the implemented versions).
> > - This->shell_link = NULL;
> > - hr = CoCreateInstance(&CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER,
> > - &IID_IShellLinkW, (LPVOID*)&This->shell_link);
> >
> >
> > - if (hr != S_OK) goto free_this;
> > -
> > - This->persist_file = NULL;
> > - IShellLinkW_QueryInterface(This->shell_link, &IID_IPersistFile,
> > - (LPVOID*)&This->persist_file);
> >
> >
> > - if (hr != S_OK) goto free_this;
>
> This is probably missing assignment to hr. For checks FAILED() is
> usually sufficient.
>
> > -
> > - hr = IPersistFile_Load(This->persist_file, item->path, STGM_READ);
> > - if (hr != S_OK) goto free_this;
> > -
> > - IShellLinkDual2_AddRef(&This->IShellLinkDual2_iface);
> > -
> > - *link = (IShellLinkDual2 *)&This->IShellLinkDual2_iface;
> > - return S_OK;
>
> Why AddRef here?
>
> > -
> > - free_this:
> > - heap_free(This);
> > - return hr;
>
> It's easy to avoid goto in this case.
More information about the wine-devel
mailing list