On Sun, 05 Oct 2014, Søren Sandmann <soren.sandm...@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 _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
