[Pixman] [PATCH 1/2] Always return a valid function from lookup_composite()

[Søren Sandmann]

Hi Chris,

These patches generally make sense to me, but I have a few comments
below.

> -    if (_pixman_implementation_lookup_composite (
> -	    imp->toplevel, info->op,
> -	    src_image->common.extended_format_code, src_flags,
> -	    mask_format, mask_flags,
> -	    dest_image->common.extended_format_code, info->dest_flags,
> -	    &imp, &func))
> +    _pixman_implementation_lookup_composite (
> +	imp->toplevel, info->op,
> +	src_image->common.extended_format_code, src_flags,
> +	mask_format, mask_flags,
> +	dest_image->common.extended_format_code, info->dest_flags,
> +	&imp, &func);
>      {

This looks like it leaves an isolated set of braces in the code.

> diff --git a/pixman/pixman-glyph.c b/pixman/pixman-glyph.c
> index 5451f42..5a271b6 100644
> --- a/pixman/pixman-glyph.c
> +++ b/pixman/pixman-glyph.c
> @@ -464,13 +464,12 @@ pixman_composite_glyphs_no_mask (pixman_op_t            op,
>  		    glyph_format = glyph_img->common.extended_format_code;
>  		    glyph_flags = glyph_img->common.flags;
>  
> -		    if (! _pixman_implementation_lookup_composite (
> -			    get_implementation(), op,
> -			    src->common.extended_format_code, src->common.flags,
> -			    glyph_format, glyph_flags | extra,
> -			    dest_format, dest_flags,
> -			    &implementation, &func))
> -			goto out;

Was this patch generated as a diff against the first patch? I don't see
"if (! _pixman_implementation_..." in the current code.

> +		    _pixman_implementation_lookup_composite (
> +			get_implementation(), op,
> +			src->common.extended_format_code, src->common.flags,
> +			glyph_format, glyph_flags | extra,
> +			dest_format, dest_flags,
> +			&implementation, &func);
>  		}
>  
>  		info.src_x = src_x + composite_box.x1 - dest_x;
> @@ -574,13 +573,12 @@ add_glyphs (pixman_glyph_cache_t *cache,
>  		white_src = TRUE;
>  	    }
>  
> -	    if (! _pixman_implementation_lookup_composite (
> -		    get_implementation(), PIXMAN_OP_ADD,
> -		    src_format, info.src_flags,
> -		    mask_format, info.mask_flags,
> -		    dest_format, dest_flags,
> -		    &implementation, &func))
> -		goto out;
> +	    _pixman_implementation_lookup_composite (
> +		get_implementation(), PIXMAN_OP_ADD,
> +		src_format, info.src_flags,
> +		mask_format, info.mask_flags,
> +		dest_format, dest_flags,
> +		&implementation, &func);
>  	}
>  
> +void
>  _pixman_implementation_lookup_composite (pixman_implementation_t  *toplevel,
>  					 pixman_op_t               op,
>  					 pixman_format_code_t      src_format,
> @@ -142,7 +148,11 @@ _pixman_implementation_lookup_composite (pixman_implementation_t  *toplevel,
>  	    ++info;
>  	}
>      }
> -    return FALSE;
> +
> +    /* We should never reach this point */
> +    _pixman_log_error (FUNC, "No known composite function\n");

I think it would be useful to have some text about TLS most likely being
broken. "No known composite function" isn't particularly informative.

> diff --git a/pixman/pixman.c b/pixman/pixman.c
> index 3fabed1..24c93c5 100644
> --- a/pixman/pixman.c
> +++ b/pixman/pixman.c
> @@ -678,10 +678,10 @@ pixman_image_composite32 (pixman_op_t      op,
>       */
>      op = optimize_operator (op, src_flags, mask_flags, dest_flags);
>  
> -    if (_pixman_implementation_lookup_composite (
> -	    get_implementation (), op,
> -	    src_format, src_flags, mask_format, mask_flags, dest_format, dest_flags,
> -	    &imp, &func))
> +    _pixman_implementation_lookup_composite (
> +	get_implementation (), op,
> +	src_format, src_flags, mask_format, mask_flags, dest_format, dest_flags,
> +	&imp, &func);
>      {
>  	pixman_composite_info_t info;
>  	const pixman_box32_t *pbox;

Another case of braces left in the code.


Søren