<div dir="ltr">On 5 September 2013 10:42, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 09/03/2013 08:28 AM, Paul Berry wrote:<br>
</div><div class="im">> On 29 August 2013 09:05, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a><br>
</div><div class="im">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote:<br>
><br>
>     From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a><br>
</div>>     <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>><br>
<br>
[snip]<br>
<div class="im"><br>
>     +interpolation_modes = {<br>
>     +    'flat',<br>
>     +    'noperspective',<br>
>     +    'smooth',<br>
>     +    'default'<br>
>     +}<br>
><br>
><br>
> This uses Python's "set literal" syntax, which was introduced in Python<br>
> 2.7.  Piglit only requires 2.6, so I think we should change this to a list:<br>
><br>
> interpolation_modes = [<br>
>     'flat',<br>
>     'noperspective',<br>
>     'smooth',<br>
>     'default'<br>
> ]<br>
><br>
> Same with vertex_shader_variables and vertex_shader_variables_front_only.<br>
><br>
> All of my other comments (below) are minor, so with this fixed, the<br>
> patch is:<br>
><br>
> Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a><br>
</div>> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>><br>
<div><div class="h5">><br>
><br>
>     +<br>
>     +vertex_shader_variables = {<br>
>     +    'gl_FrontColor',<br>
>     +    'gl_BackColor',<br>
>     +    'gl_FrontSecondaryColor',<br>
>     +    'gl_BackSecondaryColor'<br>
>     +}<br>
>     +<br>
>     +vertex_shader_variables_front_only = {<br>
>     +    'gl_FrontColor',<br>
>     +    'gl_FrontSecondaryColor',<br>
>     +}<br>
>     +<br>
>     +vertex_shader_variables_other_side = {<br>
>     +    'gl_FrontColor': 'gl_BackColor',<br>
>     +    'gl_BackColor': 'gl_FrontColor',<br>
>     +    'gl_FrontSecondaryColor': 'gl_BackSecondaryColor',<br>
>     +    'gl_BackSecondaryColor': 'gl_FrontSecondaryColor'<br>
>     +}<br>
>     +<br>
>     +fragment_shader_variables = {<br>
>     +    'gl_FrontColor': 'gl_Color',<br>
>     +    'gl_BackColor': 'gl_Color',<br>
>     +    'gl_FrontSecondaryColor': 'gl_SecondaryColor',<br>
>     +    'gl_BackSecondaryColor': 'gl_SecondaryColor'<br>
>     +}<br>
><br>
><br>
> Minor quibble: vertex_shader_variables_other_side and<br>
> fragment_shader_variables sound like names of lists, when they're in<br>
> fact mappings from one variable name to another.  Maybe rename to<br>
> something like "other_side_map" and "vs_to_fs_map"?<br>
<br>
</div></div>Should I still use the { } syntax here?  Does that work in Python 2.6?<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">Yeah, the { } syntax is fine here, since these are dict literals, which have been supported for ages.<br></div></div>