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 <nsivov(a)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.