Re: [PATCH 1/4] shell32: Add ShellLinkObject stubs.

[Myah Caron]

Thanks for the review!

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, September 13, 2020 2:09 PM, Nikolay Sivov  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.