[PATCH 0/1] MR4784: dlls/gdiplus: Clean up GdipCloneImageAttributes

[David Kahurani (＠kahurani)]

Not sure this is an improvement but it does look like one to me.

Signed-off-by: David Kahurani k.kahurani(a)gmail.com

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784

[PATCH 1/1] dlls/gdiplus: Clean up GdipCloneImageAttributes

[David Kahurani]

From: David Kahurani <k.kahurani(a)gmail.com>

This API code currently looks quite lousy with the extraneous
'if' statements and use of an unnecessary local variable.

Signed-off-by: David Kahurani <k.kahurani(a)gmail.com>
---
 dlls/gdiplus/imageattributes.c | 52 ++++++++++++++--------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/dlls/gdiplus/imageattributes.c b/dlls/gdiplus/imageattributes.c
index 349e52f5be9..5063dc92391 100644
--- a/dlls/gdiplus/imageattributes.c
+++ b/dlls/gdiplus/imageattributes.c
@@ -31,7 +31,6 @@ GpStatus WINGDIPAPI GdipCloneImageAttributes(GDIPCONST GpImageAttributes *imagea
     GpImageAttributes **cloneImageattr)
 {
     GpStatus stat = Ok;
-    struct color_remap_table remap_tables[ColorAdjustTypeCount] = {{0}};
     int i;
 
     TRACE("(%p, %p)\n", imageattr, cloneImageattr);
@@ -39,43 +38,34 @@ GpStatus WINGDIPAPI GdipCloneImageAttributes(GDIPCONST GpImageAttributes *imagea
     if(!imageattr || !cloneImageattr)
         return InvalidParameter;
 
-    for (i=0; i<ColorAdjustTypeCount; i++)
-    {
-        if (imageattr->colorremaptables[i].enabled)
-        {
-            remap_tables[i].enabled = TRUE;
-            remap_tables[i].mapsize = imageattr->colorremaptables[i].mapsize;
-            remap_tables[i].colormap = malloc(sizeof(ColorMap) * remap_tables[i].mapsize);
-
-            if (remap_tables[i].colormap)
-            {
-                memcpy(remap_tables[i].colormap, imageattr->colorremaptables[i].colormap,
-                    sizeof(ColorMap) * remap_tables[i].mapsize);
-            }
-            else
-            {
-                stat = OutOfMemory;
-                break;
-            }
-        }
-    }
-
-    if (stat == Ok)
-        stat = GdipCreateImageAttributes(cloneImageattr);
+    stat = GdipCreateImageAttributes(cloneImageattr);
 
     if (stat == Ok)
     {
         **cloneImageattr = *imageattr;
 
-        memcpy((*cloneImageattr)->colorremaptables, remap_tables, sizeof(remap_tables));
-    }
-
-    if (stat != Ok)
-    {
         for (i=0; i<ColorAdjustTypeCount; i++)
-            free(remap_tables[i].colormap);
+        {
+            if (imageattr->colorremaptables[i].enabled)
+            {
+                (*cloneImageattr)->colorremaptables[i].enabled = TRUE;
+                (*cloneImageattr)->colorremaptables[i].mapsize = imageattr->colorremaptables[i].mapsize;
+                (*cloneImageattr)->colorremaptables[i].colormap = malloc(sizeof(ColorMap) * imageattr->colorremaptables[i].mapsize);
+
+                if ((*cloneImageattr)->colorremaptables[i].colormap)
+                {
+                    memcpy((*cloneImageattr)->colorremaptables[i].colormap, imageattr->colorremaptables[i].colormap,
+			 sizeof(ColorMap) * imageattr->colorremaptables[i].mapsize);
+                }
+                else
+                {
+                    GdipDisposeImageAttributes(*cloneImageattr);
+                    stat = OutOfMemory;
+                    break;
+                }
+            }
+        }
     }
-
     return stat;
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/4784

[Esme Povirk (＠madewokherd)]

I'm not sure what criteria I should be using to compare these two versions. I don't really see the improvement.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56775

[David Kahurani (＠kahurani)]

The original version first copies the data over to a temporary variable and I, was finding the 'if' statements in the original version a bit lousy.

The proposed version copies everything directly to the clone which eliminates the temporary variable and avoids lousy 'if's.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56776

[Esme Povirk (＠madewokherd)]

I don't really see a problem with using a local variable for allocations, or with how the if statements are structured. 🤷‍♂️

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56778

[David Kahurani (＠kahurani)]

Well, of course,  there's no problem, it is more or less about what is better :shrug_tone4:‍♀️

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56779

[PATCH 0/1] MR4784: dlls/gdiplus: Clean up GdipCloneImageAttributes - closed

[Esme Povirk (＠madewokherd)]

This merge request was closed by Esme Povirk.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784

[Esme Povirk (＠madewokherd)]

I don't think it's worth changing this.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56788