[Mesa-dev] [PATCH 7/8] glsl: Replace iterators in ir_reader.cpp with ad-hoc list walking.

[Kenneth Graunke]

On 01/13/2014 09:49 AM, Ian Romanick wrote:
> On 01/11/2014 02:37 AM, Kenneth Graunke wrote:
>> These can't use foreach_list since they want to skip over the first few
>> list elements.  Just doing the ad-hoc list walking isn't too bad.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/glsl/ir_reader.cpp | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/glsl/ir_reader.cpp b/src/glsl/ir_reader.cpp
>> index f5185d2..28923f3 100644
>> --- a/src/glsl/ir_reader.cpp
>> +++ b/src/glsl/ir_reader.cpp
>> @@ -205,11 +205,12 @@ ir_reader::read_function(s_expression *expr, bool skip_body)
>>        assert(added);
>>     }
>>  
>> -   exec_list_iterator it = ((s_list *) expr)->subexpressions.iterator();
>> -   it.next(); // skip "function" tag
>> -   it.next(); // skip function name
>> -   for (/* nothing */; it.has_next(); it.next()) {
>> -      s_expression *s_sig = (s_expression *) it.get();
>> +   /* Skip over "function" tag and function name (which are guaranteed to be
>> +    * present by the above PARTIAL_MATCH call).
>> +    */
>> +   exec_node *node = ((s_list *) expr)->subexpressions.head->next->next;
>> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
>> +      s_expression *s_sig = (s_expression *) node;
> 
> This won't behave the same in the (bug) case that the list has too few
> elements.  If the list is empty or as only one element, there will be a
> NULL deref here somewhere.  I believe the iterator version was safe
> against this.
> 
> Do we have some pre-existing guarantee that the list has enough elements?

Yes.  Above:

   s_pattern pat[] = { "function", name };
   if (!PARTIAL_MATCH(expr, pat)) {
      ir_read_error(expr, "Expected (function <name> (signature ...) ...)");
      return NULL;
   }

If the list doesn't match the (partial) S-Expression

   (function <name> ...)

we would have bailed by now.  So the list is guaranteed to have at least
two elements.


>>        read_function_sig(f, s_sig, skip_body);
>>     }
>>     return added ? f : NULL;
>> @@ -249,9 +250,10 @@ ir_reader::read_function_sig(ir_function *f, s_expression *expr, bool skip_body)
>>     exec_list hir_parameters;
>>     state->symbols->push_scope();
>>  
>> -   exec_list_iterator it = paramlist->subexpressions.iterator();
>> -   for (it.next() /* skip "parameters" */; it.has_next(); it.next()) {
>> -      ir_variable *var = read_declaration((s_expression *) it.get());
>> +   /* Skip over the "parameters" tag. */
>> +   exec_node *node = paramlist->subexpressions.head->next;
>> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
>> +      ir_variable *var = read_declaration((s_expression *) node);
>>        if (var == NULL)
>>  	 return;
>>  
>>
>