[cairo] Cleanup patch for region handling

[Owen Taylor]

On Tue, 2005-08-16 at 14:35 -0700, Carl Worth wrote:
> On Tue, 16 Aug 2005 10:14:47 -0400, Owen Taylor wrote:
> > Here's a patch that splits out some existing code into utility
> > functions:
> 
> Looks pretty good. A few minor comments below.
> 
> > cairo_private void
> > _cairo_region_rectangle_extents (pixman_region16_t *region,
> >                                  cairo_rectangle_t *rect);
> 
> I'd rename that to postfix the rectangle qualifier:
> 
> 	_cairo_region_extents_rectangle
> 
> Just pretend English had postfix adjectives (adjectives postfix?) and
> this style happens automatically. ;-)

Fixed. (the code, not the English language)

> > @@ -1207,32 +1207,6 @@ _composite_trap_region_solid (cairo_clip
> >  			      cairo_surface_t       *dst,
> >  			      pixman_region16_t     *region)
> >  {
> > -    int num_rects = pixman_region_num_rects (region);
> > -    pixman_box16_t *boxes = pixman_region_rects (region);
> > -    cairo_rectangle_t *rects;
> ...
> > -    return status;
> >  }
> 
> Uhm, was that function supposed to go away, and not just its body?

Yeah, I caught that later.

> > @@ -614,15 +614,16 @@ _cairo_image_surface_composite (cairo_op
> > -    if (!_cairo_operator_bounded (operator))
> ...
> > +    if (status == CAIRO_STATUS_SUCCESS &&
> > +	!_cairo_operator_bounded (operator))
> 
> I'm confused about the handling of status in this function. Is
> _cairo_image_surface_set_attributes really supposed to return
> cairo_int_status_t rather than cairo_status_t? I don't see any
> UNSUPPORTED return values from there. Why aren't we just returning
> early on non-SUCCESS status here and instead checking it over and
> over?

The function wasn't written as goto-happy as our current style -
I was just conforming to that, but since it won't clutter a 
patch to review at this point, I'll just go ahead and fix it.

In terms of cairo_int_status_t, _cairo_pattern_acquire_surfaces() 
returns that. I don't know if it is necessary, but I'll let someone
else worry about tracking down the chain.

> > +    /* We can't use pixman_region_create_simple(), because it doesn't
> > +     * have an error return
> > +     */
> 
> That should be even easier to fix these days with the embedded
> pixman. I don't know how important this is though.

Not very ... I don't think it makes much of an efficiency difference.

One thing that would really clean things up would be to hack
pixman_region_status_t so it could be assigned to a cairo_status_t;

    if (pixman_region_subtract (clear_region, clear_region, drawn_region) != PIXMAN_REGION_STATUS_SUCCESS) {
        status = CAIRO_STATUS_NO_MEMORY;
        goto CLEANUP_REGIONS;
    }

Could become:

 status = pixman_region_subtract (clear_region, clear_region, drawn_region);
 if (status)
      goto CLEANUP_REGIONS;

I've committed this patch now along with the followup
optimization/bugfix patch. composite-integer-translate-source.c is
going to fail with this change; I'll mail shortly to summarize our
IRC discussion, the end result of it, being:

 - The reference image is wrong by the current definition of SOURCE
 - But we really should change the definition of SOURCE (again)

Regards,
						Owen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050816/2531078f/attachment.pgp