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

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 ?

>> +	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.

>> +	    _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'.


--mtx

--
Enrico Weigelt,
metux IT consulting
+49-151-27565287
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo