> On 3 Jul 2018, at 15:51, Christophe Fergeau <cferg...@redhat.com> wrote:
>
> On Tue, Jul 03, 2018 at 03:16:19PM +0200, Christophe de Dinechin wrote:
>>>> diff --git a/common/canvas_base.c b/common/canvas_base.c
>>>> index 6bf6e5d..bbaaf96 100644
>>>> --- a/common/canvas_base.c
>>>> +++ b/common/canvas_base.c
>>>> @@ -3072,6 +3072,7 @@ static void canvas_draw_stroke(SpiceCanvas
>>>> *spice_canvas, SpiceRect *bbox,
>>>> gc.tile = canvas_get_image(canvas,
>>>> stroke->brush.u.pattern.pat,
>>>> FALSE);
>>>> + spice_return_if_fail(gc.tile != NULL);
>>>
>>> I'd favour g_return_if_fail(), I'd really like to kill these various
>>> spice_* calls mirroring glib API. And spice_return_if_fail() will
>>> abort() I think.
>>
>> That’s inconsistent with the rest of the file (there are currently 47
>> instances of spice_return macros in canvas_base.c).
>>
>> What about doing a first pass that replaces all of them with
>> g_return_if_fail? That would be another patch, specifically to kill
>> that macro. I sent the patches. If they get acked before I resubmit
>> this one, then I’ll fix it here too.
>
> At least in that file, that works for me, though one has to take into
> account that we'd be changing abort() to 'return'.
>
>>
>>>
>>>> }
>>>> gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
>>>> gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
>>>> @@ -3158,6 +3159,10 @@ static void canvas_draw_rop3(SpiceCanvas
>>>> *spice_canvas, SpiceRect *bbox,
>>>> heigth = bbox->bottom - bbox->top;
>>>>
>>>> d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top,
>>>> width, heigth, FALSE);
>>>> + if (d == NULL) {
>>>> + spice_critical("no rop3 destination");
>>>
>>> spice_critical calls abort() too
>>
>> Depending on context, it may. So what? Are you suggesting to do something
>> else?
>
> if (d == NULL) {
> abort();
> free(d);
> }
> is odd, either we abort, or we cleanup and return, I would not do both.
> g_critical/g_warning would be better here.
I think we never had a discussion of what we really want in each case, and
that’s causing the confusion.
First, a meta-rule. Like you, there is a lot in SPICE code I don’t like. When
in doubt, I try to use consistency as a guide. If the code uses spice_critical
in that file, my changes will use spice_critical. That’s the case for
canvas_base.c.
Because of this consistency meta-rule, I am very much against changing on a
case-by-case basis, i.e. introducing the first g_critical, in particular
knowing that g_critical and spice_critical are not strictly equivalent. I’d
much rather do a global change so that we understand the effects. Frediano’s
recent “have you even tested this” message shows that it’s probably the correct
approach. If I can’t see a change after changing all instances of
spice_return_if_fail to g_return_if_fail, but Frediano immediately sees
something wrong to the point of “screaming", we certainly don’t want to change
only *one* of them, in particular an infrequently travelled path, because
that’s calling for
Then, on the specific point you raised. As I understand it, spice_critical
*may* abort, but it does not always abort, it depends on the abort flag.
Therefore, it is correct to free the resource in case it returns. Do you
disagree with any part of that rationale (i.e. either disagree that
spice_critical may return, or that we should free if it does)?
Finally, on removing spice_blah and move towards g_blah, I agree with the
sentiment, but as explained above, I’d rather do that as a wholesale operation
rather than piecemeal, it seems much less dangerous to me.
Thanks
Christophe
>
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
