[Pixman] [PATCH 1/2] allow runtime switching of pixman_implementation_t's for testing.

Soeren Sandmann sandmann at daimi.au.dk
Fri May 21 13:42:14 PDT 2010


Hi,

> define a new public entry point _pixman_debug_set_implementation.
> 
> _pixman_choose_implementation now takes a "char* imp_name" parameter,
> with NULL being equivalent to "general".  unrecognized names also result
> in the general implementation being chosen.
> 
> the thread local cache now includes a pixman_implementation_t* which is
> used to clear the fastpath cache when a change in pixman_implementation_t
> is noticed.

Testing implmentations against each other is a very good idea. I think
these patches are a big improvement over what we have now, so apart
from the comments below, it looks fine to go in.

- Would it be worthwhile using an environment variable along with some
  API entrypoint to make pixman recompute what implementation it is
  using? The advantage of that is that it would allow us to isolate
  bugs more quickly by asking users to reproduce it with something
  like PIXMAN_IMPLEMENTATION="general".

  I'm not sure of the answer here; there may be drawbacks to the
  environment variable that I'm missing.

- I think whatever entrypoint we end up adding should be protected
  with

        #ifdef PIXMAN_USE_INTERNAL_API

- Maybe call the fast path implementation "fast-path" just for
  consistency with the _create_fast_path constructor (though I know I
  said otherwise on IRC).

- Style comments:

  - braces go on their own line

  - multiline if/while/for bodies should have braces around them even
    if they are just one C statement.


Thanks,
Søren


More information about the Pixman mailing list