[Piglit] [PATCH 17/26] builtin_functions.py: PEP8 compliance

[Chad Versace]

On 07/10/2013 03:19 PM, Dylan Baker wrote:
> This patch creates a massive amount of churn because of a large number
> of very long lines, terrible mixed spaces/tabs indents, and
> missing/overkill whitespace.

This patch *does* create a lot of churn. Please split this patch into
multiple patches. Patch 1 will replace tabs with spaces. Patch 2 will
add additional newlines to provide sufficient PEP8-approved vertical spacing.
And subsequent patches will do more substantial formatting fixes. Such a
breakup would make these changes MUCH easier to review.

I believe everyone will agree to patch 1. I would agree with patch 2 if pacifies
the PEP8 tool. But, I don't agree with all the formatting fixes in this patch,
and would like to respond to those in your resubmission.

As PEP8 says, "know when to be inconsistent -- sometimes the style guide just doesn't apply".
I think much of this file falls under that statement.


>   # Concrete declarations of GlslBuiltinType
> -glsl_bool   = GlslBuiltinType('bool',   None,       1, 1, 110)
> -glsl_int    = GlslBuiltinType('int',    None,       1, 1, 110)
> -glsl_uint   = GlslBuiltinType('uint',   None,       1, 1, 130)
> -glsl_float  = GlslBuiltinType('float',  None,       1, 1, 110)
> -glsl_vec2   = GlslBuiltinType('vec2',   glsl_float, 1, 2, 110)
> -glsl_vec3   = GlslBuiltinType('vec3',   glsl_float, 1, 3, 110)
> -glsl_vec4   = GlslBuiltinType('vec4',   glsl_float, 1, 4, 110)
> -glsl_bvec2  = GlslBuiltinType('bvec2',  glsl_bool,  1, 2, 110)
> -glsl_bvec3  = GlslBuiltinType('bvec3',  glsl_bool,  1, 3, 110)
> -glsl_bvec4  = GlslBuiltinType('bvec4',  glsl_bool,  1, 4, 110)
> -glsl_ivec2  = GlslBuiltinType('ivec2',  glsl_int,   1, 2, 110)
> -glsl_ivec3  = GlslBuiltinType('ivec3',  glsl_int,   1, 3, 110)
> -glsl_ivec4  = GlslBuiltinType('ivec4',  glsl_int,   1, 4, 110)
> -glsl_uvec2  = GlslBuiltinType('uvec2',  glsl_uint,  1, 2, 130)
> -glsl_uvec3  = GlslBuiltinType('uvec3',  glsl_uint,  1, 3, 130)
> -glsl_uvec4  = GlslBuiltinType('uvec4',  glsl_uint,  1, 4, 130)
> -glsl_mat2   = GlslBuiltinType('mat2',   glsl_float, 2, 2, 110)
> -glsl_mat3   = GlslBuiltinType('mat3',   glsl_float, 3, 3, 110)
> -glsl_mat4   = GlslBuiltinType('mat4',   glsl_float, 4, 4, 110)
> +glsl_bool = GlslBuiltinType('bool', None, 1, 1, 110)
> +glsl_int = GlslBuiltinType('int', None, 1, 1, 110)
> +glsl_uint = GlslBuiltinType('uint', None, 1, 1, 130)
> +glsl_float = GlslBuiltinType('float', None, 1, 1, 110)
> +glsl_vec2 = GlslBuiltinType('vec2', glsl_float, 1, 2, 110)
> +glsl_vec3 = GlslBuiltinType('vec3', glsl_float, 1, 3, 110)
> +glsl_vec4 = GlslBuiltinType('vec4', glsl_float, 1, 4, 110)
> +glsl_bvec2 = GlslBuiltinType('bvec2', glsl_bool, 1, 2, 110)
> +glsl_bvec3 = GlslBuiltinType('bvec3', glsl_bool, 1, 3, 110)
> +glsl_bvec4 = GlslBuiltinType('bvec4', glsl_bool, 1, 4, 110)
> +glsl_ivec2 = GlslBuiltinType('ivec2', glsl_int, 1, 2, 110)
> +glsl_ivec3 = GlslBuiltinType('ivec3', glsl_int, 1, 3, 110)
> +glsl_ivec4 = GlslBuiltinType('ivec4', glsl_int, 1, 4, 110)
> +glsl_uvec2 = GlslBuiltinType('uvec2', glsl_uint, 1, 2, 130)
> +glsl_uvec3 = GlslBuiltinType('uvec3', glsl_uint, 1, 3, 130)
> +glsl_uvec4 = GlslBuiltinType('uvec4', glsl_uint, 1, 4, 130)
> +glsl_mat2 = GlslBuiltinType('mat2', glsl_float, 2, 2, 110)
> +glsl_mat3 = GlslBuiltinType('mat3', glsl_float, 3, 3, 110)
> +glsl_mat4 = GlslBuiltinType('mat4', glsl_float, 4, 4, 110)


The above code is in a tabular format with tabular spacing because it
is a table. Let's not alter that. The tabular spacing makes it
eminently more readable.


> @@ -189,10 +185,9 @@ glsl_mat4x4 = glsl_mat4
>   # Signature(name='step', template='step({0}, {1})',
>   #           version_introduced=110, rettype='vec3',
>   #           argtypes=('float', 'vec3'))
> -Signature = collections.namedtuple(
> -    'Signature',
> -    ('name', 'template', 'version_introduced', 'rettype', 'argtypes'))
> -
> +Signature = collections.namedtuple('Signature',
> +                                   ('name', 'template', 'version_introduced',
> +                                    'rettype', 'argtypes'))
>
>
>   # Named tuple representing a single piece of test data for testing a
> @@ -208,9 +203,8 @@ Signature = collections.namedtuple(
>   #   vectors involving booleans and integers.  If result is a vector or
>   #   matrix, tolerance should be interpreted as the maximum permissible
>   #   RMS error (as would be computed by the distance() function).
> -TestVector = collections.namedtuple(
> -    'TestVector', ('arguments', 'result', 'tolerance'))
> -
> +TestVector = collections.namedtuple('TestVector',
> +                                    ('arguments', 'result', 'tolerance'))
>


The two hunks above do adhere to the PEP8 document and they, in their
original formatting, are much more readable.



>   def _divide(x, y):
>       if any(y_element == 0 for y_element in column_major_values(y)):
> -	# Division by zero is undefined.
> -	return None
> +        # Division by zero is undefined.
> +        return None
>       if glsl_type_of(x).base_type == glsl_int:
> -	# The GLSL spec does not make it clear what the rounding rules
> -	# are when performing integer division.  C99 requires
> -	# round-toward-zero, so in the absence of any other
> -	# information, assume that's the correct behavior for GLSL.
> -	#
> -	# Python and numpy's rounding rules are inconsistent, so to
> -	# make sure we get round-toward-zero behavior, divide the
> -	# absolute values of x and y, and then fix the sign.
> -	return (np.abs(x) // np.abs(y)) * (np.sign(x) * np.sign(y))
> +        # The GLSL spec does not make it clear what the rounding rules
> +        # are when performing integer division.  C99 requires
> +        # round-toward-zero, so in the absence of any other
> +        # information, assume that's the correct behavior for GLSL.
> +        #
> +        # Python and numpy's rounding rules are inconsistent, so to
> +        # make sure we get round-toward-zero behavior, divide the
> +        # absolute values of x and y, and then fix the sign.
> +        return (np.abs(x) // np.abs(y)) * (np.sign(x) * np.sign(y))
>       elif glsl_type_of(x).base_type == glsl_uint:
> -	return x // y
> +        return x // y
>       else:
> -	return x / y
> +        return x / y
> +


The patch should not change the division operator used here.


> @@ -681,24 +690,23 @@ def _store_test_vector(test_suite_dict, name, glsl_version, test_vector,

I stopped reviewing here. The patch was just too painful to review. Please resubmit
as multiple patches.