[PATCH 0/1] MR4784: dlls/gdiplus: Clean up GdipCloneImageAttributes
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
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
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
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
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
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
This merge request was closed by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4784
I don't think it's worth changing this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4784#note_56788
participants (3)
-
David Kahurani -
David Kahurani (@kahurani) -
Esme Povirk (@madewokherd)