On 01/23/2013 02:01 PM, Paul Berry wrote: > On 22 January 2013 00:51, Ian Romanick <i...@freedesktop.org> wrote: > >> From: Ian Romanick <ian.d.roman...@intel.com> >> >> This will soon also be used for processing interface block fields. >> >> Signed-off-by: Ian Romanick <ian.d.roman...@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 <stereotype...@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.vers...@linux.intel.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev