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

[Dylan Baker]

Thanks Curro, this looks great.

Reviewed-by: Dylan Baker <dylanx.c.baker at intel.com>

On Monday, October 06, 2014 12:02:55 PM Francisco Jerez wrote:
> 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.

Okay, that's fine then

> 
> > 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.

Cool. I didn't look too closely, I just wasn't sure if you were aware of
that function.

> 
> >>[...]
> >> +#
> >> +# 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: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141006/54af88c6/attachment.sig>