[Mesa-dev] [PATCH 07/32] glsl: Refactor out processing of structure fields

[Chad Versace]

On 01/23/2013 02:01 PM, Paul Berry wrote:
> On 22 January 2013 00:51, Ian Romanick <idr at freedesktop.org> wrote:
> 
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> This will soon also be used for processing interface block fields.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/glsl/ast_to_hir.cpp | 42 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index c432369..bce3488 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -4014,35 +4014,36 @@ ast_type_specifier::hir(exec_list *instructions,
>>  }
>>
>>
>> -ir_rvalue *
>> -ast_struct_specifier::hir(exec_list *instructions,
>> -                         struct _mesa_glsl_parse_state *state)
>> +unsigned
>> +ast_process_structure_or_interface_block(exec_list *instructions,
>> +                                        struct _mesa_glsl_parse_state
>> *state,
>> +                                        exec_list *declarations,
>> +                                        YYLTYPE &loc,
>> +                                        glsl_struct_field **fields_ret)
>>
> 
> The contract with the caller isn't obvious to me from this function
> declaration.  Can we have a short comment above the function saying that
> the return value is the number of fields and that *fields_ret receives a
> pointer to a newly allocated array with that size?
> 
> With that change, this patch is:
> 
> Reviewed-by: Paul Berry <stereotype441 at gmail.com>

I find it confusing when a function with output parameters  also has a return
value, and that return value is not an error code. I like to see all the
outputs in the parameter list or all packed into the return value, not a
hybrid.

This code is correct, so you feel free to ignore my complaint (though I hope
you don't). With Paul's contract comment, this is

Reviewed-by: Chad Versace <chad.versace at linux.intel.com>