[Mesa-dev] [PATCH 6/6] glsl: Move layout(location) checks to AST-to-HIR conversion

[Paul Berry]

On 27 October 2013 14:59, Ian Romanick <idr at freedesktop.org> wrote:

> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This will simplify the addition of layout(location) qualifiers for
> separate shader objects.  This was validated with new piglit tests
> arb_explicit_attrib_location/1.30/compiler/not-enabled-01.vert and
> arb_explicit_attrib_location/1.30/compiler/not-enabled-02.vert.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/glsl/ast_to_hir.cpp | 23 +++++++++++++++++++++++
>  src/glsl/glsl_parser.yy | 34 +++++++++++++++-------------------
>  2 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 8441360..4343176 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2059,6 +2059,9 @@ validate_explicit_location(const struct
> ast_type_qualifier *qual,
>     switch (state->target) {
>     case vertex_shader:
>        if (var->mode == ir_var_shader_in) {
> +         if (!state->has_explicit_attrib_location())
> +            goto needed_explicit_attrib_location;
> +
>           break;
>        }
>
> @@ -2073,6 +2076,9 @@ validate_explicit_location(const struct
> ast_type_qualifier *qual,
>
>     case fragment_shader:
>        if (var->mode == ir_var_shader_out) {
> +         if (!state->has_explicit_attrib_location())
> +            goto needed_explicit_attrib_location;
> +
>           break;
>        }
>
> @@ -2122,6 +2128,23 @@ validate_explicit_location(const struct
> ast_type_qualifier *qual,
>           }
>        }
>     }
> +
> +   return;
> +
> +needed_explicit_attrib_location:
> +   if (state->es_shader) {
> +      _mesa_glsl_error(loc, state,
> +                       "%s explicit location requires %s",
> +                       mode_string(var),
> +                       "GLSL ES 300");
> +   } else {
> +      _mesa_glsl_error(loc, state,
> +                       "%s explicit location requires %s",
> +                       mode_string(var),
> +                       "GL_ARB_explicit_attrib_location extension or GLSL
> 330");
> +   }
> +
> +   return;
>

I'm not really a fan of the goto statements here.  We already have a
convention of adding "check_" functions which return true if something is
ok, and report an error and return false if it's not (see check_version,
check_precision_qualifiers_allowed, and check_bitwise_operations_allowed).
How about if we do a similar thing and create a
"check_explicit_attrib_location_allowed()" function.  Then the hunks above
can just say:

if (!state->check_explicit_attrib_location_allowed())
   return;

With that change, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

I sent out comments on patches 1, 2, and 4.  Patches 3 and 5 are:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>


>  }
>
>  static void
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index 0a0708e..4aacc67 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1308,29 +1308,25 @@ layout_qualifier_id:
>     {
>        memset(& $$, 0, sizeof($$));
>
> -      if (state->has_explicit_attrib_location()) {
> -         if (match_layout_qualifier("location", $1, state) == 0) {
> -            $$.flags.q.explicit_location = 1;
> +      if (match_layout_qualifier("location", $1, state) == 0) {
> +         $$.flags.q.explicit_location = 1;
>
> -            if ($3 >= 0) {
> -               $$.location = $3;
> -            } else {
> -               _mesa_glsl_error(& @3, state,
> -                                "invalid location %d specified", $3);
> -               YYERROR;
> -            }
> +         if ($3 >= 0) {
> +            $$.location = $3;
> +         } else {
> +             _mesa_glsl_error(& @3, state, "invalid location %d
> specified", $3);
> +             YYERROR;
>           }
> +      }
>
> -         if (match_layout_qualifier("index", $1, state) == 0) {
> -            $$.flags.q.explicit_index = 1;
> +      if (match_layout_qualifier("index", $1, state) == 0) {
> +         $$.flags.q.explicit_index = 1;
>
> -            if ($3 >= 0) {
> -               $$.index = $3;
> -            } else {
> -               _mesa_glsl_error(& @3, state,
> -                                "invalid index %d specified", $3);
> -               YYERROR;
> -            }
> +         if ($3 >= 0) {
> +            $$.index = $3;
> +         } else {
> +            _mesa_glsl_error(& @3, state, "invalid index %d specified",
> $3);
> +            YYERROR;
>           }
>        }
>
> --
> 1.8.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131029/669c4b3b/attachment-0001.html>