[Mesa-dev] [PATCH] glsl parser: Select typelist as early as possible.

[Kenneth Graunke]

On 04/13/2012 02:45 PM, Eric Anholt wrote:
> On Wed, 11 Apr 2012 10:56:51 +0200, Olivier Galibert<galibert at pobox.com>  wrote:
>> Type selection must be extended w.r.t version and extension lines as
>> soon as it is possible, and in any case before the lookahead is done
>> to check the nature of the next line.  Otherwise code such as:
>>
>>    #version 400
>>
>>    dmat2 function inverse(dmat2 m) { ... }
>>
>> fails because "dmat2" has already been lexed as a NEW_IDENTIFIER
>> before _mesa_glsl_initialize_types is called.
>>
>> Signed-off-by: Olivier Galibert<galibert at pobox.com>
>>
>> ---
>>   src/glsl/glsl_parser.yy |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> [Sorry, resent with the correct subject...]
>>
>> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>> index 64506b6..9a0af95 100644
>> --- a/src/glsl/glsl_parser.yy
>> +++ b/src/glsl/glsl_parser.yy
>> @@ -233,9 +233,6 @@ static void yyerror(YYLTYPE *loc, _mesa_glsl_parse_state *st, const char *msg)
>>
>>   translation_unit:
>>   	version_statement extension_statement_list
>> -	{
>> -	   _mesa_glsl_initialize_types(state);
>> -	}
>>   	external_declaration_list
>>   	{
>>   	   delete state->symbols;
>> @@ -285,6 +282,7 @@ version_statement:
>>   			       state->version_string,
>>   			       state->supported_version_string);
>>   	   }
>> +	   _mesa_glsl_initialize_types(state);
>>   	}
>>   	;
>>
>> @@ -322,6 +320,7 @@ extension_statement:
>>   	   if (!_mesa_glsl_process_extension($2,&  @2, $4,&  @4, state)) {
>>   	      YYERROR;
>>   	   }
>> +	   _mesa_glsl_initialize_types(state);
>>   	}
>
> What I was concerned about here was that we would add the same type
> multiple times when a #extension is present, and then all hell would
> break loose.  It looks like actually the type is generated and then not
> added to the symbol table when it already exists there, so this looks
> fine to me.
>
> Testing with:
>
> #version 120
> mat2x4 a;
>
> I guess that because all previous new types have been turned into
> keywords, and our handling of keywords at parse time ensures that they
> don't get treated as NEW_IDENTIFIER, this hasn't been a problem before.
> dmat2 is also a keyword.  So I'm not actually sure if this patch is
> really worth it (initializing the types in the symbol table twice, when
> you shouldn't ever be able to hit the bug it's fixing).

Yeah...we're going to have to add dmat & friends as keywords to properly 
reserve them.  It might be useful if we hit an extension that introduces 
new types...other than EXT_gpu_shader4, I can't think of one really...

I think this patch won't hurt anything, but I guess I don't see it as 
terribly useful at the moment either.