On Thu Jan 18 20:50:51 2024 +0000, Vijay Kiran Kamuju wrote:
> Here it's about the test for GdipGetEffectParameterSize() with invalid
> parameters - null effect and zero size. Hence passing a null effect.
You should pass in NULL as a constant so it's easier to tell that's what you're doing.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58025
Esme Povirk (@madewokherd) commented about dlls/gdiplus/image.c:
> + if (GdipGetEffectParameterSize(effect, ¶msize) != Ok)
> + return InvalidParameter;
> +
> + switch (effect->type)
> + {
> + case BlurEffect:
> + if (paramsize != size)
> + return InvalidParameter;
> + effect->p.blurparams = malloc(size);
> + memcpy(effect->p.blurparams, params, size);
> + break;
> + case SharpenEffect:
> + if (paramsize != size)
> + return InvalidParameter;
> + effect->p.sharpenparams = malloc(size);
> + memcpy(effect->p.sharpenparams, params, size);
I don't think it makes sense to duplicate this for each type. The member of the union may be different, but the code is equivalent.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58024
Esme Povirk (@madewokherd) commented about dlls/gdiplus/image.c:
> {
> ef = malloc(sizeof(CGpEffect));
> ef->type = ColorBalanceEffect;
> + ef->p.clrbalanceparams = NULL;
> }
> else if (IsEqualGUID(&guid, &LevelsEffectGuid))
> {
> ef = malloc(sizeof(CGpEffect));
> ef->type = LevelsEffect;
> + ef->p.levelsparams = NULL;
> }
> else if (IsEqualGUID(&guid, &ColorCurveEffectGuid))
> {
> ef = malloc(sizeof(CGpEffect));
> ef->type = ColorCurveEffect;
> + ef->p.clrcurveparams = NULL;
I don't think it makes sense to duplicate this assignment to NULL. I would just have a member of the union that is a BYTE* and use that when the type doesn't matter.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58023
On Thu Jan 18 20:41:13 2024 +0000, Esme Povirk wrote:
> effect is supposed to be NULL here, so it doesn't make sense to pass it
> in by name.
Here it's about the test for GdipGetEffectParameterSize() with invalid parameters - null effect and zero size. Hence passing a null effect.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58022
Esme Povirk (@madewokherd) commented about dlls/gdiplus/image.c:
> + else if (IsEqualGUID(&guid, &ColorBalanceEffectGuid))
> + {
> + ef = malloc(sizeof(CGpEffect));
> + ef->type = ColorBalanceEffect;
> + }
> + else if (IsEqualGUID(&guid, &LevelsEffectGuid))
> + {
> + ef = malloc(sizeof(CGpEffect));
> + ef->type = LevelsEffect;
> + }
> + else if (IsEqualGUID(&guid, &ColorCurveEffectGuid))
> + {
> + ef = malloc(sizeof(CGpEffect));
> + ef->type = ColorCurveEffect;
> + }
> + else
This could be simplified by moving the `malloc` call to after the big if statement, and storing the type in a local variable.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58019
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/image.c:
> stat = pGdipCreateEffect(BlurEffectGuid, NULL);
> expect(InvalidParameter, stat);
>
> + stat = pGdipGetEffectParameterSize(NULL, NULL);
> + expect(InvalidParameter, stat);
> +
> + stat = pGdipGetEffectParameterSize(effect, NULL);
> + expect(InvalidParameter, stat);
> +
> stat = pGdipCreateEffect(noneffect, &effect);
> expect(Win32Error, stat);
> ok(effect == NULL, "Expected effect to be NULL\n");
>
> - for(i=0; i < ARRAY_SIZE(effectlist); i++)
> + size = 0;
> + stat = pGdipGetEffectParameterSize(effect, &size);
effect is supposed to be NULL here, so it doesn't make sense to pass it in by name.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58017
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/image.c:
> expect(InvalidParameter, stat);
>
> stat = pGdipCreateEffect(noneffect, &effect);
> - todo_wine expect(Win32Error, stat);
> + expect(Win32Error, stat);
> + ok(effect == NULL, "Expected effect to be NULL\n");
effect should be initialized to non-NULL before calling GdipCreateEffect, in case it coincidentally started out as NULL.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4661#note_58016