Re: [PATCH 10/51] core: fixed code duplication

On Mon, Dec 21, 2015 at 06:01:52PM +0100, Enrico Weigelt, metux IT consult wrote:
> On 21.12.2015 13:07, Chris Wilson wrote:
> 
> Hi,
> 
> >> +/**
> >> + * copy a box set to an clip
> >> + *
> >> + * @param  box   the box set to copy from
> >> + * @param  clip  the clip to copy to (return buffer)
> >> + * @result       zero if the allocation failed - the clip will be set to all-clipped
> >> + *               otherwise non-zero
> >> + */
> >> +static int
> >> +_cairo_boxes_copy_to_clip(const cairo_boxes_t *boxes, cairo_clip_t *clip)
> >> +{
> >> +    /* XXX cow-boxes? */
> >> +    if(boxes->num_boxes == 1) {
> > 
> > Please read CODING_STYLE (specificy on whitespace and indentation).
> 
> hmm, where exactly is the violation ? the documentation header ?

I think he's referring to the lack of a space after the if.  I noticed
it too but the error was already present in the original code.  He's
right though that by style rules it needs a space here, may as well fix
it.
 
> >> +	clip->boxes = &clip->embedded_box;
> >> +	clip->boxes[0] = boxes->chunks.base[0];
> >> +	clip->num_boxes = 1;
> >> +    } else {
> >> +	clip->boxes = _cairo_boxes_to_array(boxes, &clip->num_boxes);
> >> +	if (unlikely(clip->boxes == NULL))
> > 
> > unlikely? Hmm. needs an assert(boxes->num_boxes != 0) then.
> 
> we could also check for both:
> 
>      ((clip->boxes == NULL) || (clip->num_boxes == 0))
> 
> I'd guess the compiler could generate a vector op for that, if the
> fields are direct neighbors and properly aligned.

I suppose.  What you had looked ok to me, but I'll never argue against
adding asserts().

> >> +	    _cairo_clip_set_all_clipped (clip);
> >> +	    return 0;
> >> +	}
> >> +    }
> >> +    return 1;
> >> +}
> >> +
> >>  cairo_clip_t *
> >>  _cairo_clip_intersect_boxes (cairo_clip_t *clip,
> >>  			     const cairo_boxes_t *boxes)
> >> @@ -301,13 +328,10 @@ _cairo_clip_intersect_boxes (cairo_clip_t *clip,
> >>      if (boxes->num_boxes == 0) {
> >>  	clip = _cairo_clip_set_all_clipped (clip);
> >>  	goto out;
> >> -    } else if (boxes->num_boxes == 1) {
> >> -	clip->boxes = &clip->embedded_box;
> >> -	clip->boxes[0] = boxes->chunks.base[0];
> >> -	clip->num_boxes = 1;
> >> -    } else {
> >> -	clip->boxes = _cairo_boxes_to_array (boxes, &clip->num_boxes);
> >>      }
> >> +
> >> +    _cairo_boxes_copy_to_clip(boxes, clip);
> >> +
> >>      _cairo_boxes_extents (boxes, &limits);
> >>  
> >>      _cairo_box_round_to_rectangle (&limits, &extents);
> >> @@ -574,16 +598,8 @@ _cairo_clip_from_boxes (const cairo_boxes_t *boxes)
> >>      if (clip == NULL)
> >>  	return _cairo_clip_set_all_clipped (clip);
> >>  
> >> -    /* XXX cow-boxes? */
> >> -    if(boxes->num_boxes == 1) {
> >> -	clip->boxes = &clip->embedded_box;
> >> -	clip->boxes[0] = boxes->chunks.base[0];
> >> -	clip->num_boxes = 1;
> >> -    } else {
> >> -	clip->boxes = _cairo_boxes_to_array (boxes, &clip->num_boxes);
> >> -	if (clip->boxes == NULL)
> >> -	    return _cairo_clip_set_all_clipped (clip);
> >> -    }
> >> +    if (unlikely(!_cairo_boxes_copy_to_clip(boxes, clip)))
> >> +	return 0;
> >
> > Changed the return value!
> 
> Ah, needs to be 'clip' instead of '0'.

I missed that too.

Bryce
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo