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 <kenn...@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.jus...@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
