[Pixman] [PATCH 00/32] (Mostly) ARMv6 speedups

[Ben Avison]

On Sun, 05 Oct 2014, Søren Sandmann <soren.sandmann at gmail.com> wrote:
> Comments on the patch series below. In the future, please consider
> sending smaller patch sets so that I can review them in smaller chunks
> of time. Those are easier to find than multiple consecutive hours ...

I know what you mean, I have the same problem following up your comments,
made worse by the fact that a lot of this is no longer fresh in my mind.
Unfortunately, my hands were tied, as I was trying to get as much done
as possible before an externally-imposed deadline, the release of
Raspberry Pi Foundation's WebKit-based browser Epiphany:

http://www.raspberrypi.org/web-browser-released/

Perhaps the thing to do is to thrash out each small group of related
patches one at a time. One advantage of there being so many is that I was
able to re-order them so that related patches (e.g. the benchmarking ones)
were next to each other.

Talking of which:

> ==================================================================
> 0002:	lowlevel parse				Needs work
> 0003:	affine benchmarker			Needs work
> ==================================================================
>
> Both of these could benefit from taking the support for
> format/operator parsing and printing in check-format.c and moving it
> to utils.[ch], perhaps extended to deal with strings like 'none' etc.

I don't think that makes sense for lowlevel_blt_bench. I was getting
fed up with having to manually add entries to tests_tbl[] in
lowlevel-blt-bench.c and create corresponding patches for each one.
However, I wanted to match the existing conventions of test names,
which meant matching things like "outrev" that differs from the
PIXMAN_OP_OUT_REVERSE style, and they also use short-form pixel format
names like "8888" rather than "a8r8g8b8".

There's also the important subtlety in the order of entries in
combine_type[] that strings are listed before any substrings so that
the "greediest" match is processed first. This is important because
the separator character '_' can appear both within the operator name
and between the operator and the first format.

However, since I was free to invent the command-line syntax for
affine-bench, I went with space-separated arguments that could easily be
parsed using format_from_string() and operator_from_string() so I have
changed those. I note that operator_name() and format_name() don't use
a macro like I did, so have the possibility of human error causing the
numerical and string versions to get out of step. Would you object if
I changed them so that they do use macros?

> Also some coding style issues. (Use a typedef for structs, don't
> define a struct on a single line, braces around statements that span
> multiple lines -- see CODING_STYLE for more information.

To be fair, CODING_STYLE doesn't say anything about structs, and it's
harder to spot the absence of examples of something when you're trying
to copy the existing house style.

I have updated these two patches locally and will repost when I hear your
preference about the use of macros in operator_name() and format_name().

Ben