[Mesa-dev] [PATCH 08/17] glsl parser: allow in & out for interface block members

[Kenneth Graunke]

On 04/19/2013 12:35 PM, Jordan Justen wrote:
> Previously uniform blocks allowed for the 'uniform' keyword
> to be used with members of a uniform blocks. With interface
> blocks 'in' can be used on 'in' interface block members and
> 'out' can be used on 'out' interface block members.
>
> The basic_interface_block rule will verify that the same
> qualifier type is used with the block and each member.
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>   src/glsl/glsl_parser.yy |   47 ++++++++++++++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index c254a2f..6d88263 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -2051,41 +2051,54 @@ member_list:
>   	}
>   	;
>
> -/* Specifying "uniform" inside of a uniform block is redundant. */
> -uniformopt:
> -	/* nothing */
> -	| UNIFORM
> -	;
> -
>   member_declaration:
> -	layout_qualifier uniformopt type_specifier struct_declarator_list ';'
> +	layout_qualifier fully_specified_type struct_declarator_list ';'

I don't think this is right.  The fully_specified_type production rule 
expands to "type_qualifier type_specifier".  type_qualifier expands to:

type_qualifier:
         storage_qualifier
         | layout_qualifier
         | layout_qualifier storage_qualifier

at which point you have:

layout_qualifier layout_qualifier storage_qualifier type_specifier 
struct_declarator_list ';'

I like the move to use fully_specified_type, but I think you need to 
make the rule simply:

         fully_specified_type struct_declarator_list ';'

I'm also a bit concerned that this may allow too much.  For example, is 
"invariant" allowed?  If not, are you checking that somewhere?

>   	{
>   	   void *ctx = state;
> -	   ast_fully_specified_type *type = new(ctx) ast_fully_specified_type();
> +	   ast_fully_specified_type *type = $2;
>   	   type->set_location(yylloc);
>
> -	   type->qualifier = $1;
> -	   type->qualifier.flags.q.uniform = true;
> -	   type->specifier = $3;
> +	   if (!type->qualifier.merge_qualifier(& @1, state, $1)) {
> +	      YYERROR;
> +	   }
> +
> +	   if (type->qualifier.flags.q.attribute) {
> +	      _mesa_glsl_error(& @1, state,
> +	                      "keyword 'attribute' cannot be used with "
> +	                      "interface block member\n");
> +	   } else if (type->qualifier.flags.q.varying) {
> +	      _mesa_glsl_error(& @1, state,
> +	                      "keyword 'varying' cannot be used with "
> +	                      "interface block member\n");
> +	   }
> +
>   	   $$ = new(ctx) ast_declarator_list(type);
>   	   $$->set_location(yylloc);
>   	   $$->ubo_qualifiers_valid = true;
>
> -	   $$->declarations.push_degenerate_list_at_head(& $4->link);
> +	   $$->declarations.push_degenerate_list_at_head(& $3->link);
>   	}
> -	| uniformopt type_specifier struct_declarator_list ';'
> +	| fully_specified_type struct_declarator_list ';'

The nice part is by eliminating the layout_qualifier redundancy, you can 
drop one of these two mostly-duplicated blocks :)

>   	{
>   	   void *ctx = state;
> -	   ast_fully_specified_type *type = new(ctx) ast_fully_specified_type();
> +	   ast_fully_specified_type *type = $1;
>   	   type->set_location(yylloc);
>
> -	   type->qualifier.flags.q.uniform = true;
> -	   type->specifier = $2;
> +	   if (type->qualifier.flags.q.attribute) {
> +	      _mesa_glsl_error(& @1, state,
> +	                      "keyword 'attribute' cannot be used in "
> +	                      "interface block\n");
> +	   } else if (type->qualifier.flags.q.varying) {
> +	      _mesa_glsl_error(& @1, state,
> +	                      "keyword 'varying' cannot be used in "
> +	                      "interface block\n");
> +	   }
> +
>   	   $$ = new(ctx) ast_declarator_list(type);
>   	   $$->set_location(yylloc);
>   	   $$->ubo_qualifiers_valid = true;
>
> -	   $$->declarations.push_degenerate_list_at_head(& $3->link);
> +	   $$->declarations.push_degenerate_list_at_head(& $2->link);
>   	}
>   	;
>
>