Re: [PATCH 1/2] shell32: Add a basic implementation for SHGetStockIconInfo

[Dmitry Timoshkov]

Detlef Riekenberg <wine.dev(a)web.de> wrote:
> +/****************************************************************************
> + * helper for SHGetStockIconInfo
> + */
> +typedef struct stockiconentry_t {
> +    SHSTOCKICONID id;
> +    DWORD iconid;
> +} stockiconentry;

Why bother with a typedef?
> +static stockiconentry stockicontable[] = {
> +    {SIID_DOCNOASSOC,   IDI_SHELL_DOCUMENT},
> +    {SIID_DOCASSOC,     IDI_SHELL_DOCUMENT},
> +    {SIID_FOLDER,       IDI_SHELL_FOLDER},
> +    {SIID_DRIVERNET,    IDI_SHELL_NETDRIVE},
> +    {SIID_DRIVERCD,     IDI_SHELL_CDROM},
> +    {SIID_DRIVERRAM,    IDI_SHELL_RAMDISK},
> +    {SIID_DESKTOPPC,    IDI_SHELL_MY_COMPUTER},
> +    {SIID_PRINTER,      IDI_SHELL_PRINTER},
> +    {SIID_SETTINGS,     IDI_SHELL_CONTROL_PANEL},
> +    {SIID_RECYCLERFULL, IDI_SHELL_FULL_RECYCLE_BIN},
> +    {SIID_DELETE,       IDI_SHELL_CONFIRM_DELETE},
> +};

Please don't forget to add 'const'.
> +static int cmp_stockiconentry(const void *entry1, const void *entry2)
> +{
> +    stockiconentry *p1 = (stockiconentry *) entry1;
> +    stockiconentry *p2 = (stockiconentry *) entry2;
> +
> +    return p1->id - p2->id;
> +}

Don't cast away 'const'.
> +HRESULT WINAPI SHGetStockIconInfo(SHSTOCKICONID id, UINT flags, SHSTOCKICONINFO *sii)
> +{
> +    stockiconentry *entry;
> +
> +    TRACE("(%d, 0x%x, %p)\n", id, flags, sii);
> +    if ((id < 0) | (id >= (SIID_MAX_ICONS - 1)) || !sii || (sii->cbSize != sizeof(SHSTOCKICONINFO)))
> +        return E_INVALIDARG;
> +
> +    /* find the requested icon */
> +    entry = bsearch(&id, stockicontable, sizeof(stockicontable)/sizeof(stockicontable[0]),
> +                    sizeof(stockiconentry), cmp_stockiconentry);
> +
> +    if (!entry) {
> +        FIXME("using fallback for id %d\n", id);
> +        entry = stockicontable;
> +    }
> +
> +    if (flags)
> +        FIXME("flags 0x%x not implemented\n", flags);
> +
> +    sii->hIcon = NULL;
> +    sii->iSysImageIndex = -1;
> +    sii->iIcon = - entry->iconid;
> +
> +    GetModuleFileNameW(shell32_hInstance, sii->szPath, MAX_PATH);
> +
> +    return S_OK;
> +}

GetModuleFileNameW can fail, so filling 'sii' may be a bit premature.

-- 
Dmitry.

Aw: Re: [PATCH 1/2] shell32: Add a basic implementation for SHGetStockIconInfo

[Detlef Riekenberg]

Dimitry wrote:
> > +/****************************************************************************
> > + * helper for SHGetStockIconInfo
> > + */
> > +typedef struct stockiconentry_t {
> > + SHSTOCKICONID id;
> > + DWORD iconid;
> > +} stockiconentry;
> Why bother with a typedef?

I prefer typedef to reduce a bit of typing.
Changed here, to make you happy and to feel the difference.
> > +static stockiconentry stockicontable[] = {
> > + {SIID_DOCNOASSOC, IDI_SHELL_DOCUMENT},
> Please don't forget to add 'const'.

Done. Thanks
> > +static int cmp_stockiconentry(const void *entry1, const void *entry2)
> > +{
> > + stockiconentry *p1 = (stockiconentry *) entry1;
> > + stockiconentry *p2 = (stockiconentry *) entry2;
> > +
> > + return p1->id - p2->id;
> > +}
> Don't cast away 'const'.

Fixed. Thanks
> > +HRESULT WINAPI SHGetStockIconInfo(SHSTOCKICONID id, UINT flags, SHSTOCKICONINFO *sii)
> > ...
> > + GetModuleFileNameW(shell32_hInstance, sii->szPath, MAX_PATH);
> GetModuleFileNameW can fail, so filling 'sii' may be a bit premature.

What is the failure case???
The module-handle, the target buffer pointer and the buffer size are valid.
A path longer as MAX_PATH for shell32.dll is not supported in windows.

-- 
By by ... Detlef

[PATCH 1/2] shell32: Add a basic implementation for SHGetStockIconInfo

[Henri Verbeet]

On 8 February 2013 11:11, Detlef Riekenberg <wine.dev(a)web.de> wrote:
> I prefer typedef to reduce a bit of typing.

Typedefs aren't meant to be a tool for obfuscation. (Though if you're
into that, there are some interesting things you can make the
preprocessor do as well.)