From: Serge Gautherie <reactos-git_serge_fake@gautherie.fr>
Date: Tue, 04 Apr 2017 22:02:00 +0200
Subject: [SHELL32]
iconcache.cpp:
*A few rewrites.
*Add/Improve some debug messages.
*SIC_Initialize() failure code: Review if()s, Fix destroying ShellBigIconList.
*SIC_Destroy(): Review if()s.
CORE-13033


Index: reactos/dll/win32/shell32/iconcache.cpp
--- a/reactos/dll/win32/shell32/iconcache.cpp
+++ b/reactos/dll/win32/shell32/iconcache.cpp
@@ -35,8 +35,7 @@
     DWORD dwAccessTime;
 } SIC_ENTRY, * LPSIC_ENTRY;
 
-static HDPA        sic_hdpa = 0;
-
+static HDPA sic_hdpa;
 static HIMAGELIST ShellSmallIconList;
 static HIMAGELIST ShellBigIconList;
 
@@ -127,14 +126,10 @@
                               resource id, but not all icons are present yet
                               so we can't use icon indices */
 
-    if (s_imgListIdx != -1)
-    {
-        if (large)
-            ShortcutIcon = ImageList_GetIcon(ShellBigIconList, s_imgListIdx, ILD_TRANSPARENT);
-        else
-            ShortcutIcon = ImageList_GetIcon(ShellSmallIconList, s_imgListIdx, ILD_TRANSPARENT);
-    } else
-        ShortcutIcon = NULL;
+    ShortcutIcon = (s_imgListIdx == -1)
+      ? NULL
+      : ImageList_GetIcon((large ? ShellBigIconList : ShellSmallIconList),
+                          s_imgListIdx, ILD_TRANSPARENT);
 
     if (!ShortcutIcon || !GetIconInfo(ShortcutIcon, &ShortcutIconInfo))
     {
@@ -502,6 +497,7 @@
     sic_hdpa = DPA_Create(16);
     if (!sic_hdpa)
     {
+        ERR("Failed to create icon cache.\n");
         return FALSE;
     }
 
@@ -513,6 +509,7 @@
     }
 
     bpp = GetDeviceCaps(hDC, BITSPIXEL);
+
     DeleteDC(hDC);
 
     if (bpp <= 4)
@@ -527,7 +524,6 @@
         ilMask = ILC_COLOR32;
     else
         ilMask = ILC_COLOR;
-
     ilMask |= ILC_MASK;
 
     cx_small = GetSystemMetrics(SM_CXSMICON);
@@ -582,14 +578,15 @@
         goto end;
     }
 
-    if(SIC_IconAppend(swShell32Name, IDI_SHELL_DOCUMENT-1, hSm, hLg, 0) == INVALID_INDEX)
+    if (SIC_IconAppend(swShell32Name, IDI_SHELL_DOCUMENT - 1, hSm, hLg, 0) == INVALID_INDEX)
     {
-        ERR("Failed to add IDI_SHELL_DOCUMENT icon to cache.\n");
+        ERR("Failed to add 'IDI_SHELL_DOCUMENT - 1' icon to cache.\n");
         goto end;
     }
+
     if(SIC_IconAppend(swShell32Name, -IDI_SHELL_DOCUMENT, hSm, hLg, 0) == INVALID_INDEX)
     {
-        ERR("Failed to add IDI_SHELL_DOCUMENT icon to cache.\n");
+        ERR("Failed to add '-IDI_SHELL_DOCUMENT' icon to cache.\n");
         goto end;
     }
     
@@ -604,12 +601,27 @@
     /* Clean everything if something went wrong */
     if(!result)
     {
-        if(sic_hdpa) DPA_Destroy(sic_hdpa);
-        if(ShellSmallIconList) ImageList_Destroy(ShellSmallIconList);
-        if(ShellBigIconList) ImageList_Destroy(ShellSmallIconList);
+        if (ShellBigIconList)
+        {
+            ImageList_Destroy(ShellBigIconList);
+            ShellBigIconList = NULL;
+        }
+
+        if (ShellSmallIconList)
+        {
+            ImageList_Destroy(ShellSmallIconList);
+            ShellSmallIconList = NULL;
+        }
+
+        // NB: A null sic_hdpa would have already returned.
+/*
+ * Q: Should this DPA_Destroy() be DPA_DestroyCallback(sic_hdpa, sic_free, NULL) here too? (Given previous SIC_IconAppend() calls...)
+ * Q: If yes, could/should we simply call SIC_Destroy()?
+ * Q: Also, should (whole) SIC_Initialize() use xxxCriticalSection(&SHELL32_SicCS)?
+ * *: Anyway, should we document what SHELL32_SicCS is actually protecting? (DPA_*() calls? sic_hdpa? ...?)
+*/
+        DPA_Destroy(sic_hdpa);
         sic_hdpa = NULL;
-        ShellSmallIconList = NULL;
-        ShellBigIconList = NULL;
     }
 
     TRACE("hIconSmall=%p hIconBig=%p\n",ShellSmallIconList, ShellBigIconList);
@@ -631,18 +643,28 @@
 
 void SIC_Destroy(void)
 {
-    TRACE("\n");
+    TRACE("Entered SIC_Destroy\n");
 
     EnterCriticalSection(&SHELL32_SicCS);
 
-    if (sic_hdpa) DPA_DestroyCallback(sic_hdpa, sic_free, NULL );
+    if (ShellBigIconList)
+    {
+        ImageList_Destroy(ShellBigIconList);
+        ShellBigIconList = NULL;
+    }
 
-    sic_hdpa = NULL;
-    ImageList_Destroy(ShellSmallIconList);
-    ShellSmallIconList = 0;
-    ImageList_Destroy(ShellBigIconList);
-    ShellBigIconList = 0;
+    if (ShellSmallIconList)
+    {
+        ImageList_Destroy(ShellSmallIconList);
+        ShellSmallIconList = NULL;
+    }
 
+    if (sic_hdpa)
+    {
+        DPA_DestroyCallback(sic_hdpa, sic_free, NULL);
+        sic_hdpa = NULL;
+    }
+
     LeaveCriticalSection(&SHELL32_SicCS);
     //DeleteCriticalSection(&SHELL32_SicCS); //static
 }