[Mesa-dev] [PATCH V2 2/2] glsl: Add check for unsized arrays to glsl types
Paul Berry
stereotype441 at gmail.com
Mon Oct 28 14:08:00 CET 2013
On 23 October 2013 03:31, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> The main purpose of this patch is to increase readability of
> the array code by introducing is_unsized_array() to glsl_types.
> Some redundent is_array() checks are also removed, and small number
> of other related clean ups.
>
> The introduction of is_unsized_array() should also make the
> ARB_arrays_of_arrays code simpler and more readable when it arrives.
>
> V2: Also replace code that checks for unsized arrays directly with the
> length variable
>
> Signed-off-by: Timothy Arceri <t_arceri at yahoo.com.au>
>
Nice clean-up, thanks. With the minor fixes noted below, both patches are:
Reviewed-by: Paul Berry <stereotype441 at gmail.com>
I've pushed them upstream.
> ---
> src/glsl/ast_array_index.cpp | 2 +-
> src/glsl/ast_function.cpp | 11 +++++------
> src/glsl/ast_to_hir.cpp | 24 ++++++++++--------------
> src/glsl/glsl_types.h | 11 ++++++++---
> src/glsl/hir_field_selection.cpp | 2 +-
> src/glsl/linker.cpp | 4 ++--
> src/glsl/opt_array_splitting.cpp | 2 +-
> 7 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
> index 107c29e..f7b5e83 100644
> --- a/src/glsl/ast_array_index.cpp
> +++ b/src/glsl/ast_array_index.cpp
> @@ -165,7 +165,7 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
> if (array->type->is_array())
> update_max_array_access(array, idx, &loc, state);
> } else if (const_index == NULL && array->type->is_array()) {
> - if (array->type->array_size() == 0) {
> + if (array->type->is_unsized_array()) {
> _mesa_glsl_error(&loc, state, "unsized array index must be
> constant");
> } else if (array->type->fields.array->is_interface()
> && array->variable_referenced()->mode == ir_var_uniform)
> {
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 02aad4f..5efd691 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -732,21 +732,20 @@ process_array_constructor(exec_list *instructions,
> exec_list actual_parameters;
> const unsigned parameter_count =
> process_parameters(instructions, &actual_parameters, parameters,
> state);
> + bool is_unsized_array = constructor_type->is_unsized_array();
>
> - if ((parameter_count == 0)
> - || ((constructor_type->length != 0)
> + if ((parameter_count == 0) || (!is_unsized_array
> && (constructor_type->length != parameter_count))) {
>
Formatting nit pick: where possible we try to break lines where the
parenthesis nesting is as little as possible, so that we can line up the
text after the line break at the column after the enclosing opening paren.
I've reformmated this to:
if ((parameter_count == 0) ||
(!is_unsized_array && (constructor_type->length !=
parameter_count))) {
> - const unsigned min_param = (constructor_type->length == 0)
> - ? 1 : constructor_type->length;
> + const unsigned min_param = is_unsized_array ? 1 :
> constructor_type->length;
>
Formatting nit pick: we try to keep to <80 columns. I've reformatted this
to:
const unsigned min_param = is_unsized_array
? 1 : constructor_type->length;
>
> _mesa_glsl_error(loc, state, "array constructor must have %s %u "
> "parameter%s",
> - (constructor_type->length == 0) ? "at least" :
> "exactly",
> + is_unsized_array ? "at least" : "exactly",
> min_param, (min_param <= 1) ? "" : "s");
> return ir_rvalue::error_value(ctx);
> }
>
> - if (constructor_type->length == 0) {
> + if (is_unsized_array) {
> constructor_type =
> glsl_type::get_array_instance(constructor_type->element_type(),
> parameter_count);
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 12be2fd..abe6059 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -651,16 +651,15 @@ validate_assignment(struct _mesa_glsl_parse_state
> *state,
> if (rhs->type == lhs_type)
> return rhs;
>
> - /* If the array element types are the same and the size of the LHS is
> zero,
> + /* If the array element types are the same and the LHS is unsized,
> * the assignment is okay for initializers embedded in variable
> * declarations.
> *
> * Note: Whole-array assignments are not permitted in GLSL 1.10, but
> this
> * is handled by ir_dereference::is_lvalue.
> */
> - if (is_initializer && lhs_type->is_array() && rhs->type->is_array()
> - && (lhs_type->element_type() == rhs->type->element_type())
> - && (lhs_type->array_size() == 0)) {
> + if (is_initializer && lhs_type->is_unsized_array() &&
> rhs->type->is_array()
> + && (lhs_type->element_type() == rhs->type->element_type())) {
> return rhs;
> }
>
> @@ -767,7 +766,7 @@ do_assignment(exec_list *instructions, struct
> _mesa_glsl_parse_state *state,
> * dereference of a variable. Any other case would require that
> the LHS
> * is either not an l-value or not a whole array.
> */
> - if (lhs->type->array_size() == 0) {
> + if (lhs->type->is_unsized_array()) {
> ir_dereference *const d = lhs->as_dereference();
>
> assert(d != NULL);
> @@ -2355,8 +2354,7 @@ get_variable_being_redeclared(ir_variable *var,
> YYLTYPE loc,
> * later re-declare the same name as an array of the same
> * type and specify a size."
> */
> - if ((earlier->type->array_size() == 0)
> - && var->type->is_array()
> + if (earlier->type->is_unsized_array() && var->type->is_array()
> && (var->type->element_type() == earlier->type->element_type())) {
> /* FINISHME: This doesn't match the qualifiers on the two
> * FINISHME: declarations. It's not 100% clear whether this is
> @@ -2608,7 +2606,7 @@ handle_geometry_shader_input_decl(struct
> _mesa_glsl_parse_state *state,
> return;
> }
>
> - if (var->type->length == 0) {
> + if (var->type->is_unsized_array()) {
> /* Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec
> says:
> *
> * All geometry shader input unsized array declarations will be
> @@ -3257,7 +3255,7 @@ ast_declarator_list::hir(exec_list *instructions,
> const glsl_type *const t = (earlier == NULL)
> ? var->type : earlier->type;
>
> - if (t->is_array() && t->length == 0)
> + if (t->is_unsized_array())
> /* Section 10.17 of the GLSL ES 1.00 specification states that
> * unsized array declarations have been removed from the
> language.
> * Arrays that are sized using an initializer are still
> explicitly
> @@ -3390,7 +3388,7 @@ ast_parameter_declarator::hir(exec_list
> *instructions,
> type = process_array_type(&loc, type, this->array_size, state);
> }
>
> - if (!type->is_error() && type->array_size() == 0) {
> + if (!type->is_error() && type->is_unsized_array()) {
> _mesa_glsl_error(&loc, state, "arrays passed as parameters must
> have "
> "a declared size");
> type = glsl_type::error_type;
> @@ -3562,7 +3560,7 @@ ast_function::hir(exec_list *instructions,
> * "Arrays are allowed as arguments and as the return type. In both
> * cases, the array must be explicitly sized."
> */
> - if (return_type->is_array() && return_type->length == 0) {
> + if (return_type->is_unsized_array()) {
> YYLTYPE loc = this->get_location();
> _mesa_glsl_error(& loc, state,
> "function `%s' return type array must be explicitly
> "
> @@ -5016,10 +5014,8 @@ ast_gs_input_layout::hir(exec_list *instructions,
> /* Note: gl_PrimitiveIDIn has mode ir_var_shader_in, but it's not an
> * array; skip it.
> */
> - if (!var->type->is_array())
> - continue;
>
> - if (var->type->length == 0) {
> + if (var->type->is_unsized_array()) {
> if (var->max_array_access >= num_vertices) {
> _mesa_glsl_error(&loc, state,
> "this geometry shader input layout implies
> %u"
> diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> index e60c191..177d863 100644
> --- a/src/glsl/glsl_types.h
> +++ b/src/glsl/glsl_types.h
> @@ -468,7 +468,6 @@ struct glsl_type {
> : error_type;
> }
>
> -
> /**
> * Get the type of a structure field
> *
> @@ -478,13 +477,11 @@ struct glsl_type {
> */
> const glsl_type *field_type(const char *name) const;
>
> -
> /**
> * Get the location of a filed within a record type
> */
> int field_index(const char *name) const;
>
> -
> /**
> * Query the number of elements in an array type
> *
>
We try to keep whitespace cleanups in separate patches. I've moved these
to their own patch.
> @@ -499,6 +496,14 @@ struct glsl_type {
> }
>
> /**
> + * Query whether the array size for all dimensions has been declared.
> + */
> + bool is_unsized_array() const
> + {
> + return is_array() && length == 0;
> + }
> +
> + /**
> * Return the number of coordinate components needed for this sampler
> type.
> *
> * This is based purely on the sampler's dimensionality. For example,
> this
> diff --git a/src/glsl/hir_field_selection.cpp
> b/src/glsl/hir_field_selection.cpp
> index 08be743..1e92c89 100644
> --- a/src/glsl/hir_field_selection.cpp
> +++ b/src/glsl/hir_field_selection.cpp
> @@ -72,7 +72,7 @@ _mesa_ast_field_selection_to_hir(const ast_expression
> *expr,
> _mesa_glsl_error(&loc, state, "length method takes no
> arguments");
>
> if (op->type->is_array()) {
> - if (op->type->array_size() == 0)
> + if (op->type->is_unsized_array())
> _mesa_glsl_error(&loc, state, "length called on unsized
> array");
>
> result = new(ctx) ir_constant(op->type->array_size());
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 0a949b4..01b7a60 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1105,7 +1105,7 @@ private:
> */
> static void fixup_type(const glsl_type **type, unsigned
> max_array_access)
> {
> - if ((*type)->is_array() && (*type)->length == 0) {
> + if ((*type)->is_unsized_array()) {
> *type = glsl_type::get_array_instance((*type)->fields.array,
> max_array_access + 1);
> assert(*type != NULL);
> @@ -1120,7 +1120,7 @@ private:
> {
> for (unsigned i = 0; i < type->length; i++) {
> const glsl_type *elem_type = type->fields.structure[i].type;
> - if (elem_type->is_array() && elem_type->length == 0)
> + if (elem_type->is_unsized_array())
> return true;
> }
> return false;
> diff --git a/src/glsl/opt_array_splitting.cpp
> b/src/glsl/opt_array_splitting.cpp
> index 34ac836..c7c5f67 100644
> --- a/src/glsl/opt_array_splitting.cpp
> +++ b/src/glsl/opt_array_splitting.cpp
> @@ -132,7 +132,7 @@
> ir_array_reference_visitor::get_variable_entry(ir_variable *var)
> /* If the array hasn't been sized yet, we can't split it. After
> * linking, this should be resolved.
> */
> - if (var->type->is_array() && var->type->length == 0)
> + if (var->type->is_unsized_array())
> return NULL;
>
> foreach_iter(exec_list_iterator, iter, this->variable_list) {
> --
> 1.8.3.1
>
> _______________________________________________
> 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/20131028/91525c9d/attachment-0001.html>
More information about the mesa-dev
mailing list