Re: [Mesa-dev] [PATCH 08/14] glsl: refactor duplicated validations between 2 layout-qualifiers

[Andres Gomez]

On Fri, 2016-11-25 at 11:07 +1100, Timothy Arceri wrote:
> Thanks.

Thanks for your patience with the review! ☺

> Patches 7 & 8 are:
> 
> Reviewed-by: Timothy Arceri 

Pushing!

-- 
Br,

Andres
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/14] glsl: refactor duplicated validations between 2 layout-qualifiers

[Timothy Arceri]

Thanks.

Patches 7 & 8 are:

Reviewed-by: Timothy Arceri 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 08/14] glsl: refactor duplicated validations between 2 layout-qualifiers

[Andres Gomez]

Several layout-qualifier validations are duplicated in the
merge_qualifier and validate_in_qualifier methods.

We would rather have them refactored into single calls.

Suggested by Timothy.

Signed-off-by: Andres Gomez 
---
 src/compiler/glsl/ast_type.cpp | 126 +
 1 file changed, 76 insertions(+), 50 deletions(-)

diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 1adb1f4..20c0fcf 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -116,6 +116,72 @@ bool ast_type_qualifier::has_memory() const
   || this->flags.q.write_only;
 }
 
+static bool
+validate_prim_type(YYLTYPE *loc,
+   _mesa_glsl_parse_state *state,
+   const ast_type_qualifier ,
+   const ast_type_qualifier _qualifier)
+{
+   /* Input layout qualifiers can be specified multiple
+* times in separate declarations, as long as they match.
+*/
+   if (qualifier.flags.q.prim_type && new_qualifier.flags.q.prim_type
+   && qualifier.prim_type != new_qualifier.prim_type) {
+  _mesa_glsl_error(loc, state,
+   "conflicting input primitive %s specified",
+   state->stage == MESA_SHADER_GEOMETRY ?
+   "type" : "mode");
+  return false;
+   }
+
+   return true;
+}
+
+static bool
+validate_vertex_spacing(YYLTYPE *loc,
+_mesa_glsl_parse_state *state,
+const ast_type_qualifier ,
+const ast_type_qualifier _qualifier)
+{
+   if (qualifier.flags.q.vertex_spacing && new_qualifier.flags.q.vertex_spacing
+   && qualifier.vertex_spacing != new_qualifier.vertex_spacing) {
+  _mesa_glsl_error(loc, state,
+   "conflicting vertex spacing specified");
+  return false;
+   }
+
+   return true;
+}
+
+static bool
+validate_ordering(YYLTYPE *loc,
+  _mesa_glsl_parse_state *state,
+  const ast_type_qualifier ,
+  const ast_type_qualifier _qualifier)
+{
+   if (qualifier.flags.q.ordering && new_qualifier.flags.q.ordering
+   && qualifier.ordering != new_qualifier.ordering) {
+  _mesa_glsl_error(loc, state,
+   "conflicting ordering specified");
+  return false;
+   }
+
+   return true;
+}
+
+static bool
+validate_point_mode(YYLTYPE *loc,
+_mesa_glsl_parse_state *state,
+const ast_type_qualifier ,
+const ast_type_qualifier _qualifier)
+{
+   /* Point mode can only be true if the flag is set. */
+   assert (!qualifier.flags.q.point_mode || !new_qualifier.flags.q.point_mode
+   || (qualifier.point_mode && new_qualifier.point_mode));
+
+   return true;
+}
+
 /**
  * This function merges both duplicate identifies within a single layout and
  * multiple layout qualifiers on a single variable declaration. The
@@ -127,6 +193,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
 const ast_type_qualifier ,
 bool is_single_layout_merge)
 {
+   bool r = true;
ast_type_qualifier ubo_mat_mask;
ubo_mat_mask.flags.i = 0;
ubo_mat_mask.flags.q.row_major = 1;
@@ -196,13 +263,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
}
 
if (q.flags.q.prim_type) {
-  if (this->flags.q.prim_type && this->prim_type != q.prim_type) {
- _mesa_glsl_error(loc, state,
-  "conflicting input primitive %s specified",
-  state->stage == MESA_SHADER_GEOMETRY ?
-  "type" : "mode");
- return false;
-  }
+  r &= validate_prim_type(loc, state, *this, q);
   this->flags.q.prim_type = 1;
   this->prim_type = q.prim_type;
}
@@ -298,26 +359,19 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
}
 
if (q.flags.q.vertex_spacing) {
-  if (this->flags.q.vertex_spacing && this->vertex_spacing != 
q.vertex_spacing) {
- _mesa_glsl_error(loc, state, "conflicting vertex spacing used");
- return false;
-  }
+  r &= validate_vertex_spacing(loc, state, *this, q);
   this->flags.q.vertex_spacing = 1;
   this->vertex_spacing = q.vertex_spacing;
}
 
if (q.flags.q.ordering) {
-  if (this->flags.q.ordering && this->ordering != q.ordering) {
- _mesa_glsl_error(loc, state, "conflicting ordering used");
- return false;
-  }
+  r &= validate_ordering(loc, state, *this, q);
   this->flags.q.ordering = 1;
   this->ordering = q.ordering;
}
 
if (q.flags.q.point_mode) {
-  /* Point mode can only be true if the flag is set. */
-  assert (!this->flags.q.point_mode || (this->point_mode && q.point_mode));
+  r &= validate_point_mode(loc, state, *this, q);
   this->flags.q.point_mode = 1;
   this->point_mode = q.point_mode;
}
@@