Looks good to me,
Reviewed-by: Iago Toral Quiroga <ito...@igalia.com>
On Thu, 2016-02-11 at 15:45 +1100, Timothy Arceri wrote:
> Commit c98deb18d5836f in 2010 disallowed embedded struct definitions
> in ES. Then in 2013 d9bb8b7b56ce65b disallowed it for everything but
> GLSL 1.10.
>
> Commit c98deb18d5836f seemed the cleanest way to do the check so its
> been extended to cover GL and the other version has been removed.
>
> Cc: Ian Romanick <ian.d.roman...@intel.com>
> Cc: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/compiler/glsl/ast_to_hir.cpp | 59
> +++++++++-----------------------
> src/compiler/glsl/glsl_parser_extras.cpp | 2 --
> src/compiler/glsl/glsl_parser_extras.h | 7 ----
> 3 files changed, 17 insertions(+), 51 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index f8f5d21..3840cba 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -6301,13 +6301,24 @@ ast_process_struct_or_iface_block_members(exec_list
> *instructions,
>
> decl_list->type->specifier->hir(instructions, state);
>
> - /* Section 10.9 of the GLSL ES 1.00 specification states that
> - * embedded structure definitions have been removed from the language.
> + /* Section 4.1.8 (Structures) of the GLSL 1.10 spec says:
> + *
> + * "Anonymous structures are not supported; so embedded structures
> + * must have a declarator. A name given to an embedded struct is
> + * scoped at the same level as the struct it is embedded in."
> + *
> + * The same section of the GLSL 1.20 spec says:
> + *
> + * "Anonymous structures are not supported. Embedded structures are
> + * not supported."
> + *
> + * The GLSL ES 1.00 and 3.00 specs have similar langauge. So, we allow
> + * embedded structures in 1.10 only.
> */
> - if (state->es_shader && decl_list->type->specifier->structure != NULL)
> {
> - _mesa_glsl_error(&loc, state, "embedded structure definitions are "
> - "not allowed in GLSL ES 1.00");
> - }
> + if (state->language_version != 110 &&
> + decl_list->type->specifier->structure != NULL)
> + _mesa_glsl_error(&loc, state,
> + "embedded structure declarations are not allowed");
>
> const glsl_type *decl_type =
> decl_list->type->glsl_type(& type_name, state);
> @@ -6626,33 +6637,6 @@ ast_struct_specifier::hir(exec_list *instructions,
> {
> YYLTYPE loc = this->get_location();
>
> - /* Section 4.1.8 (Structures) of the GLSL 1.10 spec says:
> - *
> - * "Anonymous structures are not supported; so embedded structures
> must
> - * have a declarator. A name given to an embedded struct is scoped at
> - * the same level as the struct it is embedded in."
> - *
> - * The same section of the GLSL 1.20 spec says:
> - *
> - * "Anonymous structures are not supported. Embedded structures are
> not
> - * supported.
> - *
> - * struct S { float f; };
> - * struct T {
> - * S; // Error: anonymous structures disallowed
> - * struct { ... }; // Error: embedded structures disallowed
> - * S s; // Okay: nested structures with name are
> allowed
> - * };"
> - *
> - * The GLSL ES 1.00 and 3.00 specs have similar langauge and examples.
> So,
> - * we allow embedded structures in 1.10 only.
> - */
> - if (state->language_version != 110 && state->struct_specifier_depth != 0)
> - _mesa_glsl_error(&loc, state,
> - "embedded structure declarations are not allowed");
> -
> - state->struct_specifier_depth++;
> -
> unsigned expl_location = 0;
> if (layout && layout->flags.q.explicit_location) {
> if (!process_qualifier_constant(state, &loc, "location",
> @@ -6696,8 +6680,6 @@ ast_struct_specifier::hir(exec_list *instructions,
> }
> }
>
> - state->struct_specifier_depth--;
> -
> /* Structure type definitions do not have r-values.
> */
> return NULL;
> @@ -6817,11 +6799,6 @@ ast_interface_block::hir(exec_list *instructions,
> exec_list declared_variables;
> glsl_struct_field *fields;
>
> - /* Treat an interface block as one level of nesting, so that embedded
> struct
> - * specifiers will be disallowed.
> - */
> - state->struct_specifier_depth++;
> -
> /* For blocks that accept memory qualifiers (i.e. shader storage), verify
> * that we don't have incompatible qualifiers
> */
> @@ -6885,8 +6862,6 @@ ast_interface_block::hir(exec_list *instructions,
> expl_location,
> expl_align);
>
> - state->struct_specifier_depth--;
> -
> if (!redeclaring_per_vertex) {
> validate_identifier(this->block_name, loc, state);
>
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> b/src/compiler/glsl/glsl_parser_extras.cpp
> index 20ec89d..99a0428 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -69,8 +69,6 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct
> gl_context *_ctx,
> this->error = false;
> this->loop_nesting_ast = NULL;
>
> - this->struct_specifier_depth = 0;
> -
> this->uses_builtin_functions = false;
>
> /* Set default language version and extensions */
> diff --git a/src/compiler/glsl/glsl_parser_extras.h
> b/src/compiler/glsl/glsl_parser_extras.h
> index a905b56..4dacc2a 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -290,13 +290,6 @@ struct _mesa_glsl_parse_state {
> gl_shader_stage stage;
>
> /**
> - * Number of nested struct_specifier levels
> - *
> - * Outside a struct_specifier, this is zero.
> - */
> - unsigned struct_specifier_depth;
> -
> - /**
> * Default uniform layout qualifiers tracked during parsing.
> * Currently affects uniform blocks and uniform buffer variables in
> * those blocks.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
