[Pixman] [PATCH 3/5] test: Macroise name<->number operator and format lookups

[Pekka Paalanen]

On Tue,  3 Mar 2015 15:24:18 +0000
Ben Avison <bavison at riscosopen.org> wrote:

> This reduces code size and eliminates the possibility of cut-and-paste errors
> when extending the enums. It also highlighted a mismatch between the set of
> formats accepted by format_name() and format_from_string(). For now I have
> left the mismatch in place.
> ---
>  test/utils.c |  359 +++++++++++++++++++++++-----------------------------------
>  1 files changed, 144 insertions(+), 215 deletions(-)
> 
> diff --git a/test/utils.c b/test/utils.c
> index efec632..e350878 100644
> --- a/test/utils.c
> +++ b/test/utils.c
> @@ -29,6 +29,15 @@
>  #include <png.h>
>  #endif
>  
> +typedef struct
> +{
> +    const char *name;
> +    uint32_t    number;
> +} name_to_number_t;
> +
> +#define CAT(x,y) x##y
> +#define NAME_TO_NUMBER_INITIALIZER(prefix,name) { #name, CAT(prefix,name) }
> +
>  /* Random number generator state
>   */
>  
> @@ -949,106 +958,119 @@ initialize_palette (pixman_indexed_t *palette, uint32_t depth, int is_rgb)
>      }
>  }
>  
> -static const pixman_op_t op_list[] =
> -{
> -    PIXMAN_OP_CLEAR,
> -    PIXMAN_OP_SRC,
> -    PIXMAN_OP_DST,
> -    PIXMAN_OP_OVER,
> -    PIXMAN_OP_OVER_REVERSE,
> -    PIXMAN_OP_IN,
> -    PIXMAN_OP_IN_REVERSE,
> -    PIXMAN_OP_OUT,
> -    PIXMAN_OP_OUT_REVERSE,
> -    PIXMAN_OP_ATOP,
> -    PIXMAN_OP_ATOP_REVERSE,
> -    PIXMAN_OP_XOR,
> -    PIXMAN_OP_ADD,
> -    PIXMAN_OP_SATURATE,
> -
> -    PIXMAN_OP_DISJOINT_CLEAR,
> -    PIXMAN_OP_DISJOINT_SRC,
> -    PIXMAN_OP_DISJOINT_DST,
> -    PIXMAN_OP_DISJOINT_OVER,
> -    PIXMAN_OP_DISJOINT_OVER_REVERSE,
> -    PIXMAN_OP_DISJOINT_IN,
> -    PIXMAN_OP_DISJOINT_IN_REVERSE,
> -    PIXMAN_OP_DISJOINT_OUT,
> -    PIXMAN_OP_DISJOINT_OUT_REVERSE,
> -    PIXMAN_OP_DISJOINT_ATOP,
> -    PIXMAN_OP_DISJOINT_ATOP_REVERSE,
> -    PIXMAN_OP_DISJOINT_XOR,
> -
> -    PIXMAN_OP_CONJOINT_CLEAR,
> -    PIXMAN_OP_CONJOINT_SRC,
> -    PIXMAN_OP_CONJOINT_DST,
> -    PIXMAN_OP_CONJOINT_OVER,
> -    PIXMAN_OP_CONJOINT_OVER_REVERSE,
> -    PIXMAN_OP_CONJOINT_IN,
> -    PIXMAN_OP_CONJOINT_IN_REVERSE,
> -    PIXMAN_OP_CONJOINT_OUT,
> -    PIXMAN_OP_CONJOINT_OUT_REVERSE,
> -    PIXMAN_OP_CONJOINT_ATOP,
> -    PIXMAN_OP_CONJOINT_ATOP_REVERSE,
> -    PIXMAN_OP_CONJOINT_XOR,
> -
> -    PIXMAN_OP_MULTIPLY,
> -    PIXMAN_OP_SCREEN,
> -    PIXMAN_OP_OVERLAY,
> -    PIXMAN_OP_DARKEN,
> -    PIXMAN_OP_LIGHTEN,
> -    PIXMAN_OP_COLOR_DODGE,
> -    PIXMAN_OP_COLOR_BURN,
> -    PIXMAN_OP_HARD_LIGHT,
> -    PIXMAN_OP_SOFT_LIGHT,
> -    PIXMAN_OP_DIFFERENCE,
> -    PIXMAN_OP_EXCLUSION,
> -    PIXMAN_OP_HSL_HUE,
> -    PIXMAN_OP_HSL_SATURATION,
> -    PIXMAN_OP_HSL_COLOR,
> -    PIXMAN_OP_HSL_LUMINOSITY
> +static const name_to_number_t op_list[] =
> +{
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CLEAR),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_SRC),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DST),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_OVER),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_OVER_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_IN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_IN_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_OUT),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_OUT_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_ATOP),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_ATOP_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_XOR),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_ADD),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_SATURATE),
> +
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_CLEAR),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_SRC),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_DST),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_OVER),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_OVER_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_IN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_IN_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_OUT),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_OUT_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_ATOP),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_ATOP_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DISJOINT_XOR),
> +
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_CLEAR),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_SRC),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_DST),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_OVER),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_OVER_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_IN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_IN_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_OUT),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_OUT_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_ATOP),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_ATOP_REVERSE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_CONJOINT_XOR),
> +
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_MULTIPLY),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_SCREEN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_OVERLAY),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DARKEN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_LIGHTEN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_COLOR_DODGE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_COLOR_BURN),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_HARD_LIGHT),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_SOFT_LIGHT),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_DIFFERENCE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_EXCLUSION),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_HSL_HUE),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_HSL_SATURATION),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_HSL_COLOR),
> +    NAME_TO_NUMBER_INITIALIZER(,PIXMAN_OP_HSL_LUMINOSITY),
>  };

Hi,

this is mostly a stylistic issue and I wonder what the maintainers
think...

NAME_TO_NUMBER_INITIALIZER is very long to read IMO, I've seen some
projects #define _(...) and then #undef _ right after the array is
initialized. Would that be appropriate for Pixman?

(Please read my reply to patch 4 first.)

If we need some aliases, this would need to change somehow to allow
them nicely. Wonder if we should just record two strings per entry:
 { PIXMAN_OP_OVER_REVERSE, "PIXMAN_OP_OVER_REVERSE", "over_reverse" },
 { PIXMAN_OP_OVER_REVERSE, "PIXMAN_OP_OVER_REVERSE", "overrev" },
so that operator_name () can just return the first string and
operator_from_string () can attempt to match both.

Eh, we should just see what needs to be done to patch 4, and then think
about this.

Nice find about the format mismatches. After this patch series there is:

> const char *
> format_name (pixman_format_code_t format)
> {
>     /* First check for one of the formats in format_list */
>     int i;
> 
>     for (i = 0; i < ARRAY_LENGTH (format_list); ++i)
>     {
>         if (format == format_list[i].number)
>             return format_list[i].name;
>     }
> 
>     /* Also permit some additional formats */
>     switch (format)
>     {
> /* 8bpp formats */
> #if 0
>     case PIXMAN_x4c4: return "x4c4";
>     case PIXMAN_g8: return "g8";
> #endif
>     case PIXMAN_c8: return "x4c4 / c8";
>     case PIXMAN_x4g4: return "x4g4 / g8";
> 
> /* 4bpp formats */
>     case PIXMAN_c4: return "c4";
>     case PIXMAN_g4: return "g4";
> 
> /* 1bpp formats */
>     case PIXMAN_g1: return "g1";
> 
> /* YUV formats */
>     case PIXMAN_yuy2: return "yuy2";
>     case PIXMAN_yv12: return "yv12";
> 
>     default: break;
>     };

Wonder if those should be added to the format table in a follow-up or
not.


Thanks,
pq