[Piglit] [PATCH 06/25] arb_shader_image_load_store: Import GLSL parser test generator script.

[Francisco Jerez]

Dylan Baker <baker.dylan.c at gmail.com> writes:

> I have a couple of style nits and one request for you if you wouldn't
> mind. I don't need to see a v2 since I think these are very simple.
>

Thanks for your comments Dylan :)

> 1) Could you put two newlines between toplevel functions, that is our
> style. (PEP8 style)

OK, fixed that.

> 2) Could you put your constant data before your function definitions,
> again, that's PEP8

I don't think that's going to work, my constant "data" references my
function definitions.

> 3) could you not put a space between the brace and first character in
> lists and dicts, same as above.
> 4) Could you use the print_function from the __future__ module instead
> of the print statement, I've spent a bunch of time converting the rest
> of the python code to use the print_function already.
>
These two should be fixed now as well, see attachment.

> I have a few comments, below, but all of those are just suggestions
>
> Otherwise this looks pretty straight forward and decent.
> With those changes, for the python code you have my r-b.
>
> Reviewed-by: Dylan Baker <dylanx.c.baker at intel.com>
>
> On Monday, October 06, 2014 12:00:36 AM Francisco Jerez wrote:
>>[...]
>> +def product(ps, *qss):
>> +    """
>> +    Generate the cartesian product of a number of lists of dictionaries.
>> +
>> +    Each generated element will be the union of some combination of
>> +    elements from the iterable arguments.  The resulting value of each
>> +    'name' item will be the concatenation of names of the respective
>> +    element combination separated with dashes.
>> +    """
>> +    for q in (product(*qss) if qss else [{}]):
>> +        for p in ps:
>> +            r = dict(p, **q)
>> +            r['name'] = '-'.join(s['name'] for s in (p, q) if s.get('name'))
>> +            yield r
>
> Is itertools.product() insufficient for this task?
>
It's not sufficient by itself, it returns ordered tuples of elements and
we need each combination to be merged into a single dictionary with the
somewhat ad-hoc handling of the 'name' attribute.  Sure, we could call
itertools.product() and transform the returned items as they're
generated, but I find the result slightly less readable than the naive
implementation.

>>[...]
>> +#
>> +# Test the preprocessor defines.
>> +#
>> +gen('preprocessor', """
>
> you might consider using """\ instead of just """ so you don't get a
> newline at the start of each test file.
>

Fixed that, thanks.

>>[...]

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-arb_shader_image_load_store-Import-GLSL-parser-test-.patch
Type: text/x-diff
Size: 26031 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141006/e1327ed6/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141006/e1327ed6/attachment-0001.sig>