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

[Jordan Justen]

I agree with all your suggestions on this patch. With them, is this r-b you?

-Jordan

On Sun, Apr 21, 2013 at 11:31 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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;
>>         }
>>         ;
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev