[Mesa-dev] [PATCH 11/17] nir: Trivial clean ups in the generated nir_constant_expressions.c

[Ian Romanick]

On 08/27/2015 06:41 AM, Jason Ekstrand wrote:
> 
> On Aug 27, 2015 4:08 AM, "Ian Romanick" <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>>
>> On 08/26/2015 10:11 PM, Jason Ekstrand wrote:
>> > On Wed, Aug 26, 2015 at 10:20 AM, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>> >> From: Ian Romanick <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>
>> >>
>> >> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>
>> >> ---
>> >>  src/glsl/nir/nir_constant_expressions.py | 13 +++++--------
>> >>  1 file changed, 5 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/src/glsl/nir/nir_constant_expressions.py
> b/src/glsl/nir/nir_constant_expressions.py
>> >> index e2feff3..099bb77 100644
>> >> --- a/src/glsl/nir/nir_constant_expressions.py
>> >> +++ b/src/glsl/nir/nir_constant_expressions.py
>> >> @@ -226,7 +226,6 @@ static nir_const_value
>> >>  evaluate_${name}(unsigned num_components, nir_const_value *_src)
>> >>  {
>> >>     nir_const_value _dst_val = { { {0, 0, 0, 0} } };
>> >> -
>> >
>> > I'd keep the blank line
>> >
>> >>     ## For each non-per-component input, create a variable srcN that
>> >>     ## contains x, y, z, and w elements which are filled in with the
>> >>     ## appropriately-typed values.
>> >> @@ -238,7 +237,7 @@ evaluate_${name}(unsigned num_components,
> nir_const_value *_src)
>> >>           <% continue %>
>> >>        %endif
>> >>
>> >> -   struct ${op.input_types[j]}_vec src${j} = {
>> >> +   const struct ${op.input_types[j]}_vec src${j} = {
>> >>        % for k in range(op.input_sizes[j]):
>> >>           % if op.input_types[j] == "bool":
>> >>        _src[${j}].u[${k}] != 0,
>> >> @@ -280,17 +279,17 @@ evaluate_${name}(unsigned num_components,
> nir_const_value *_src)
>> >>                 ## Avoid unused variable warnings
>> >>                 <% continue %>
>> >>              % elif op.input_types[j] == "bool":
>> >> -      bool src${j} = _src[${j}].u[_i] != 0;
>> >> +      const bool src${j} = _src[${j}].u[_i] != 0;
>> >>              % else:
>> >> -      ${op.input_types[j]} src${j} =
> _src[${j}].${op.input_types[j][:1]}[_i];
>> >> +      const ${op.input_types[j]} src${j} =
> _src[${j}].${op.input_types[j][:1]}[_i];
>> >>              % endif
>> >>           % endfor
>> >> -
>> >
>> > Same here.  Other than that, I like the cleanup.  With the blank
> lines kept,
>>
>> You should look at the generated code.  To me, the spurious blank lines
>> are the most objectionable part.  If someone submitted C code formatted
>> that way, we'd tell them to go fix it.  Right?
> 
> The generator, not the generated C code, is what's checked into the repo
> and is what people will be editing. I'd far rather keep that clean. 
> Again just run it through indent if it bothers you.

What, exactly, do the spurious blank lines in the generator add to
readability?  Having the C code in the generator consistently formatted
_as C code in this project_ and they Mako consistently indented as Mako
seems much more logical... otherwise you have to actually go look at the
generated code to see what the flow of the generated C code is intended
to be.  That's just weird.

Also like I said before... we have NEVER used indent during the build
except in cases where it was absolutely necessary.  I see no reason to
start now.

>> > Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com
> <mailto:jason.ekstrand at intel.com>>
>> >
>> >>           ## Create an appropriately-typed variable dst and assign the
>> >>           ## result of the const_expr to it.  If const_expr already
> contains
>> >>           ## writes to dst, just include const_expr directly.
>> >>           % if "dst" in op.const_expr:
>> >>        ${op.output_type} dst;
>> >> +
>> >>        ${op.const_expr}
>> >>           % else:
>> >>        ${op.output_type} dst = ${op.const_expr};
>> >> @@ -344,10 +343,8 @@ nir_eval_const_opcode(nir_op op, unsigned
> num_components,
>> >>  {
>> >>     switch (op) {
>> >>  % for name in sorted(opcodes.iterkeys()):
>> >> -   case nir_op_${name}: {
>> >> +   case nir_op_${name}:
>> >>        return evaluate_${name}(num_components, src);
>> >> -      break;
>> >> -   }
>> >>  % endfor
>> >>     default:
>> >>        unreachable("shouldn't get here");
>> >> --
>> >> 2.1.0
>> >>
>> >> _______________________________________________
>> >> mesa-dev mailing list
>> >> mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev