Re: [Mesa-dev] [PATCH 1/3] glsl: remove duplicate embedded struct validation

[Iago Toral]

Looks good to me,

Reviewed-by: Iago Toral Quiroga 

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 
> Cc: Kenneth Graunke 
> ---
>  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(, 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(, 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(, 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, , "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) {
>   

[Mesa-dev] [PATCH 1/3] glsl: remove duplicate embedded struct validation

[Timothy Arceri]

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 
Cc: Kenneth Graunke 
---
 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(, 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(, 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(, 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, , "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,