[Mesa-dev] [PATCH 03/11] glsl: Parse the "binding" keyword and store it in ast_type_qualifier.

[Kenneth Graunke]

On 07/18/2013 11:39 AM, Paul Berry wrote:
> On 17 July 2013 18:24, Kenneth Graunke <kenneth at whitecape.org
> <mailto:kenneth at whitecape.org>> wrote:
>
>     Nothing actually uses this yet.
>
>     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>>
>     ---
>       src/glsl/ast.h          | 14 ++++++++++++++
>       src/glsl/ast_type.cpp   |  6 +++++-
>       src/glsl/glsl_parser.yy | 12 ++++++++++++
>       3 files changed, 31 insertions(+), 1 deletion(-)
>
>     diff --git a/src/glsl/ast.h b/src/glsl/ast.h
>     index 6aede00..d98f1a3 100644
>     --- a/src/glsl/ast.h
>     +++ b/src/glsl/ast.h
>     @@ -413,6 +413,12 @@ struct ast_type_qualifier {
>                */
>               unsigned explicit_index:1;
>
>     +         /**
>     +          * Flag set if GL_ARB_shading_language_420pack "binding"
>     layout
>     +          * qualifier is used.
>     +          */
>     +         unsigned explicit_binding:1;
>     +
>                /** \name Layout qualifiers for GL_AMD_conservative_depth */
>                /** \{ */
>                unsigned depth_any:1;
>     @@ -456,6 +462,14 @@ struct ast_type_qualifier {
>          int index;
>
>          /**
>     +    * Binding specified via GL_ARB_shading_language_420pack's
>     "binding" keyword.
>     +    *
>     +    * \note
>     +    * This field is only valid if \c explicit_binding is set.
>     +    */
>     +   int binding;
>     +
>     +   /**
>           * Return true if and only if an interpolation qualifier is
>     present.
>           */
>          bool has_interpolation() const;
>     diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
>     index 4cbb835..275b2a1 100644
>     --- a/src/glsl/ast_type.cpp
>     +++ b/src/glsl/ast_type.cpp
>     @@ -71,7 +71,8 @@ ast_type_qualifier::has_layout() const
>                 || this->flags.q.row_major
>                 || this->flags.q.packed
>                 || this->flags.q.explicit_location
>     -          || this->flags.q.explicit_index;
>     +          || this->flags.q.explicit_index
>     +          || this->flags.q.explicit_binding;
>       }
>
>       bool
>     @@ -145,6 +146,9 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>          if (q.flags.q.explicit_index)
>             this->index = q.index;
>
>     +   if (q.flags.q.explicit_binding)
>     +      this->binding = q.binding;
>     +
>          return true;
>       }
>
>     diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>     index 3867cf8..013b327 100644
>     --- a/src/glsl/glsl_parser.yy
>     +++ b/src/glsl/glsl_parser.yy
>     @@ -1254,6 +1254,18 @@ layout_qualifier_id:
>                }
>             }
>
>     +      if (state->ARB_shading_language_420pack_enable &&
>     +          strcmp("binding", $1) == 0) {
>     +         $$.flags.q.explicit_binding = 1;
>     +
>     +         if ($3 >= 0) {
>
>
> Isn't this redundant with the check performed by
> validate_binding_qualifier (which is added in patch 4)?  I'd rather not
> do the same check in two places, and I have a minor preference to drop
> it from here and do it only in validate_binding_qualifier.

Yes, mostly.  The YYERROR here causes it to stop parsing as well, but I 
don't really know if that's a feature.  All the other layout qualifiers 
enforce the >= 0 check here in the parser, so I did likewise for symmetry.

But I do prefer handling all the range checking in one place, so I'm 
happy to remove this and just keep the check in 
validate_binding_qualifier().

>     +            $$.binding = $3;
>     +         } else {
>     +            _mesa_glsl_error(& @3, state, "invalid binding %d
>     specified\n", $3);
>     +            YYERROR;
>     +         }
>     +      }
>     +
>             /* If the identifier didn't match any known layout identifiers,
>              * emit an error.
>              */
>     --
>     1.8.3.2
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>