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

[Dmitry Timoshkov]

Detlef Riekenberg  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.