``` +/**************************************************************************** + * StockIconIdToResourceId [SHELL32.@] + * + * Map the SHSTOCKICONID value to shresdef.h IDI_ values + */ ``` Those "shell32.@" annotations are used for exports, which this isn't. This comment isn't adding anything at all and I would remove it. ``` +INT WINAPI StockIconIdToResourceId(SHSTOCKICONID id) ``` This should be static, should not be WINAPI, and can we please use a name that's more in line with the usual internal code style and doesn't look like like it's an API function? "stock_icon_id_to_resource_id" would be better. ``` +#define MISSING_ICON(x) case x : FIXME("Missing stock icon %s\n", #x); return -1 ``` Why bother with these? I don't think it's that important to have a string printed, we can always look it up later. ``` + default: FIXME("Undefined stock icon id\n"); return -1; ``` And then in the actual default case you don't print the ID, which is the one thing that's necessary... ``` - if (flags) - FIXME("flags 0x%x not implemented\n", flags); + if (flags & ~SHGSI_ICON) + FIXME("unhandled flags 0x%x\n", flags); ``` Why change the message? ``` - if (flags & SHGSI_ICON) - sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(sii->iIcon)); + if (flags & SHGSI_ICON) { + sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(StockIconIdToResourceId(id))); + } ``` Why add braces? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110838