On 27 October 2013 14:59, Ian Romanick <i...@freedesktop.org> wrote:
> From: Ian Romanick <ian.d.roman...@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.roman...@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 <stereotype...@gmail.com>
I sent out comments on patches 1, 2, and 4. Patches 3 and 5 are:
Reviewed-by: Paul Berry <stereotype...@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@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
