[Mesa-dev] [PATCH 6/8] glsl: Use a new foreach_list2 macro for walking two lists at once.

[Kenneth Graunke]

On 01/13/2014 09:58 AM, Ian Romanick wrote:
> On 01/11/2014 02:37 AM, Kenneth Graunke wrote:
>> When handling function calls, we often want to walk through the list of
>> formal parameters and list of actual parameters at the same time.
>> (Both are guaranteed to be the same length.)
>>
>> Previously, we used a pattern of:
>>
>>    exec_list_iterator 1st_iter = <1st list>.iterator();
>>    foreach_iter(exec_list_iterator, 2nd_iter, <2nd list>) {
>>       ...
>>       1st_iter.next();
>>    }
>>
>> This was a bit awkward, since you had to manually iterate through one of
>> the two lists.
> 
> "a bit"  lol.
> 
>> This patch introduces a foreach_list2 macro which safely walks through
>> two lists at the same time, so you can simply do:
>>
>>    foreach_list2(1st_node, <1st list>, 2nd_node, <2nd list>) {
>>       ...
>>    }
> 
> My only suggestion might be to change the name to foreach_two_lists.  I
> think it's more obvious to someone reading the header file looking for
> utility macros.

Yeah, that is better.  Renamed in v2.  Thanks!

>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/glsl/ast_function.cpp                  | 16 ++++----------
>>  src/glsl/ir.cpp                            | 12 +++-------
>>  src/glsl/linker.cpp                        |  9 ++++----
>>  src/glsl/list.h                            | 16 ++++++++++++++
>>  src/glsl/opt_constant_folding.cpp          |  9 ++++----
>>  src/glsl/opt_constant_propagation.cpp      |  9 ++++----
>>  src/glsl/opt_constant_variable.cpp         |  9 ++++----
>>  src/glsl/opt_copy_propagation.cpp          |  9 ++++----
>>  src/glsl/opt_copy_propagation_elements.cpp |  9 ++++----
>>  src/glsl/opt_function_inlining.cpp         | 35 ++++++++++++------------------
>>  src/glsl/opt_tree_grafting.cpp             | 10 ++++-----
>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 22 +++++++------------
>>  12 files changed, 73 insertions(+), 92 deletions(-)
>>
>> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
>> index e4c0fd1..9a9bb74 100644
>> --- a/src/glsl/ast_function.cpp
>> +++ b/src/glsl/ast_function.cpp
>> @@ -293,15 +293,10 @@ generate_call(exec_list *instructions, ir_function_signature *sig,
>>      * call takes place.  Since we haven't emitted the call yet, we'll place
>>      * the post-call conversions in a temporary exec_list, and emit them later.
>>      */
>> -   exec_list_iterator actual_iter = actual_parameters->iterator();
>> -   exec_list_iterator formal_iter = sig->parameters.iterator();
>> -
>> -   while (actual_iter.has_next()) {
>> -      ir_rvalue *actual = (ir_rvalue *) actual_iter.get();
>> -      ir_variable *formal = (ir_variable *) formal_iter.get();
>> -
>> -      assert(actual != NULL);
>> -      assert(formal != NULL);
>> +   foreach_list2(formal_node, &sig->parameters,
>> +                 actual_node, actual_parameters) {
>> +      ir_rvalue *actual = (ir_rvalue *) actual_node;
>> +      ir_variable *formal = (ir_variable *) formal_node;
> 
> The old code asserts when the lists aren't the same length... or at
> least when sig->parameters is shorter than actual_parameters.  As do the
> loops in st_glsl_to_tgsi.cpp.  I think a debug-build version of
> foreach_list2 could do the same... I'm just waffling whether there's
> sufficient value to make it worth doing.  Opinions?

I'd rather not.  These lists are always the same length.  It might be
worth checking that when creating them, but making every code site that
walks them assert seems like overkill.

Plus, it seems tricky to shoehorn assertions into a macro that only
defines a for loop (without the body).  And right now, it has the
defined behavior that it stops at the shorter of the two lists, which
could be useful someday.

--Ken