[Mesa-dev] [PATCH 06/17] glsl parser: handle interface block member qualifier

[Kenneth Graunke]

On 04/19/2013 12:35 PM, Jordan Justen wrote:
> An interface block member may specify the type:
> in {
>      in vec4 in_var_with_qualifier;
> };
>
> When specified with the member, it must match the same
> type as interface block type.
>
> It can also omit the qualifier:
> uniform {
>      vec4 uniform_var_without_qualifier;
> };
>
> When the type is not specified with the member,
> it will adopt the same type as the interface block.
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>   src/glsl/glsl_parser.yy |   26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)

This code looks correct, but I feel it could use some more comments and 
a bit more tidying.  Suggestions below:

> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index 70764d6..c254a2f 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1958,8 +1958,34 @@ basic_interface_block:
>   	                "an instance name are not allowed");
>   	   }
>
> +	   unsigned interface_type_mask, interface_type_flags;
> +	   struct ast_type_qualifier temp_type_qualifier;
> +

/* Get a bitmask containing only the in/out/uniform flags, allowing us
  * to ignore other irrelevant flags like interpolation qualifiers.
  */

> +	   temp_type_qualifier.flags.i = 0;
> +	   temp_type_qualifier.flags.q.uniform = true;
> +	   temp_type_qualifier.flags.q.in = true;
> +	   temp_type_qualifier.flags.q.out = true;
> +	   interface_type_mask = temp_type_qualifier.flags.i;
> +	   interface_type_flags = $1.flags.i & interface_type_mask;

This variable's name is confusing, for two reasons:

1. The name is plural, even though it always contains exactly one flag:
    $1 (the interface_qualifier production) returns in, out, or uniform.

    This also means you don't need to mask it by interface_type_mask,
    though it's harmless to do so.

2. The similar naming and juxtaposition seems to strongly associate the
    interface_type_mask and interface_type_flags variables.  However,
    they serve entirely different purposes.  interface_type_flags is
    actually the single interface qualifier on the whole block.

Perhaps this instead:

    /* Get the block's interface qualifier.  The interface_qualifier
     * production rule guarantees that only one bit will be set (and
     * it will be in/out/uniform).
     */
    unsigned block_interface_qualifier = $1.flags.i;

If you want, you could assert(_mesa_bitcount(block_interface_qualifier) 
== 1), but I'm not sure that function is conveniently available here. 
Not sure it's worth the hassle to get it.

>   	   block->layout.flags.i |= $1.flags.i;

This could simply be:

    block->layout.flags.i |= block_interface_qualifier;

>
> +	   foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {
> +	      ast_type_qualifier& qualifier = member->type->qualifier;
> +	      if ((qualifier.flags.i & interface_type_mask) == 0) {

/* The block member has no explicit interface qualifier.
  * Populate it based on the block's overall qualifier.
  */

(this makes it clear that you want the AST node to have the appropriate 
flag explicitly set on each variable, rather than looking it up 
implicitly from the parent block.)

If you want a spec quote you could use:
  * From the GLSL 1.50.11 specification, section 4.3.7:
  * "If no optional qualifier is used in a member declaration, the
  *  qualifier of the variable is just in, out, or uniform as declared
  *  by interface-qualifier."
but I don't think it needs it.

> +	         qualifier.flags.i |= interface_type_flags;
> +	      } else if ((qualifier.flags.i & interface_type_mask) !=
> +	                 interface_type_flags) {

I'd quote the sentence before this text:
/* The variable's qualifier doesn't match the block qualifier.
  * From the GLSL 1.50.11 spec, section 4.3.7 (Interface Blocks):
  * "If optional qualifiers are used, they can include interpolation
  *  and storage qualifiers and they must declare an input, output,
  *  or uniform variable consistent with the interface qualifier of
  *  the block."
  */

In particular, the the "consistent with the block" text makes it clear 
what you're doing here.  I'm fine with quoting both if you prefer.

> +	         /* GLSLangSpec.1.50.11, 4.3.7 Interface Blocks:
> +	          * "Input variables, output variables, and uniform variables can only
> +	          *  be in in blocks, out blocks, and uniform blocks, respectively."
> +	          */
> +	         _mesa_glsl_error(& @1, state,
> +	                          "uniform/in/out qualifier on "
> +	                          "interface block member does not match "
> +	                          "the interface block\n");
> +	      }
> +	   }
> +
>   	   $$ = block;
>   	}
>   	;
>