Oof... that's a giant wall of text. :( I skimmed this patch, and it
mostly looks right. I made a couple comments below. I've pruned out
hunks that did not have comments.
On 04/11/2014 03:52 AM, Iago Toral Quiroga wrote:
> ---
> src/glsl/ast_to_hir.cpp | 1344
> +++++++++++++++++++++++------------------------
> 1 file changed, 668 insertions(+), 676 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index b70b628..9760534 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -792,30 +792,29 @@ do_assignment(exec_list *instructions, struct
> _mesa_glsl_parse_state *state,
> if (non_lvalue_description != NULL) {
> _mesa_glsl_error(&lhs_loc, state,
> "assignment to %s",
> - non_lvalue_description);
> - error_emitted = true;
> + non_lvalue_description);
> + error_emitted = true;
> } else if (lhs->variable_referenced() != NULL
> - && lhs->variable_referenced()->data.read_only) {
> + && lhs->variable_referenced()->data.read_only) {
> _mesa_glsl_error(&lhs_loc, state,
> "assignment to read-only variable '%s'",
> lhs->variable_referenced()->name);
> error_emitted = true;
> -
> } else if (lhs->type->is_array() &&
> !state->check_version(120, 300, &lhs_loc,
> "whole array assignment forbidden")) {
> - /* From page 32 (page 38 of the PDF) of the GLSL 1.10 spec:
> - *
> - * "Other binary or unary expressions, non-dereferenced
> - * arrays, function names, swizzles with repeated fields,
> - * and constants cannot be l-values."
> - *
> - * The restriction on arrays is lifted in GLSL 1.20 and GLSL ES
> 3.00.
> - */
> - error_emitted = true;
> + /* From page 32 (page 38 of the PDF) of the GLSL 1.10 spec:
> + *
> + * "Other binary or unary expressions, non-dereferenced
> + * arrays, function names, swizzles with repeated fields,
> + * and constants cannot be l-values."
> + *
> + * The restriction on arrays is lifted in GLSL 1.20 and GLSL ES 3.00.
> + */
> + error_emitted = true;
This indentation looks fishy. It seems like the comment block should be
indented more. Just looking at the patch is hard...
> } else if (!lhs->is_lvalue()) {
> - _mesa_glsl_error(& lhs_loc, state, "non-lvalue in assignment");
> - error_emitted = true;
> + _mesa_glsl_error(& lhs_loc, state, "non-lvalue in assignment");
> + error_emitted = true;
> }
> }
>
> @@ -830,24 +829,24 @@ do_assignment(exec_list *instructions, struct
> _mesa_glsl_parse_state *state,
> * is either not an l-value or not a whole array.
> */
> if (lhs->type->is_unsized_array()) {
> - ir_dereference *const d = lhs->as_dereference();
> + ir_dereference *const d = lhs->as_dereference();
>
> - assert(d != NULL);
> + assert(d != NULL);
>
> - ir_variable *const var = d->variable_referenced();
> + ir_variable *const var = d->variable_referenced();
>
> - assert(var != NULL);
> + assert(var != NULL);
>
> - if (var->data.max_array_access >= unsigned(rhs->type->array_size())) {
> - /* FINISHME: This should actually log the location of the RHS. */
> - _mesa_glsl_error(& lhs_loc, state, "array size must be > %u due to "
> - "previous access",
> - var->data.max_array_access);
> - }
> + if (var->data.max_array_access >=
> unsigned(rhs->type->array_size())) {
> + /* FINISHME: This should actually log the location of the RHS. */
> + _mesa_glsl_error(& lhs_loc, state, "array size must be > %u due
> to "
> + "previous access",
> + var->data.max_array_access);
This also looks wrong. The later lines should match with the (.
I'm going to copy-and-paste this comment other places...
> + }
>
> - var->type = glsl_type::get_array_instance(lhs->type->element_type(),
> - rhs->type->array_size());
> - d->type = var->type;
> + var->type = glsl_type::get_array_instance(lhs->type->element_type(),
> + rhs->type->array_size());
> + d->type = var->type;
> }
> if (lhs->type->is_array()) {
> mark_whole_array_access(rhs);
> @@ -951,19 +950,19 @@ do_comparison(void *mem_ctx, int operation, ir_rvalue
> *op0, ir_rvalue *op1)
>
> case GLSL_TYPE_ARRAY: {
> for (unsigned int i = 0; i < op0->type->length; i++) {
> - ir_rvalue *e0, *e1, *result;
> -
> - e0 = new(mem_ctx) ir_dereference_array(op0->clone(mem_ctx, NULL),
> - new(mem_ctx) ir_constant(i));
> - e1 = new(mem_ctx) ir_dereference_array(op1->clone(mem_ctx, NULL),
> - new(mem_ctx) ir_constant(i));
> - result = do_comparison(mem_ctx, operation, e0, e1);
> -
> - if (cmp) {
> - cmp = new(mem_ctx) ir_expression(join_op, cmp, result);
> - } else {
> - cmp = result;
> - }
> + ir_rvalue *e0, *e1, *result;
> +
> + e0 = new(mem_ctx) ir_dereference_array(op0->clone(mem_ctx, NULL),
> + new(mem_ctx) ir_constant(i));
> + e1 = new(mem_ctx) ir_dereference_array(op1->clone(mem_ctx, NULL),
> + new(mem_ctx) ir_constant(i));
This also looks wrong. The later lines should match with the (.
> + result = do_comparison(mem_ctx, operation, e0, e1);
> +
> + if (cmp) {
> + cmp = new(mem_ctx) ir_expression(join_op, cmp, result);
> + } else {
> + cmp = result;
> + }
> }
>
> mark_whole_array_access(op0);
> @@ -973,20 +972,20 @@ do_comparison(void *mem_ctx, int operation, ir_rvalue
> *op0, ir_rvalue *op1)
>
> case GLSL_TYPE_STRUCT: {
> for (unsigned int i = 0; i < op0->type->length; i++) {
> - ir_rvalue *e0, *e1, *result;
> - const char *field_name = op0->type->fields.structure[i].name;
> -
> - e0 = new(mem_ctx) ir_dereference_record(op0->clone(mem_ctx, NULL),
> - field_name);
> - e1 = new(mem_ctx) ir_dereference_record(op1->clone(mem_ctx, NULL),
> - field_name);
> - result = do_comparison(mem_ctx, operation, e0, e1);
> -
> - if (cmp) {
> - cmp = new(mem_ctx) ir_expression(join_op, cmp, result);
> - } else {
> - cmp = result;
> - }
> + ir_rvalue *e0, *e1, *result;
> + const char *field_name = op0->type->fields.structure[i].name;
> +
> + e0 = new(mem_ctx) ir_dereference_record(op0->clone(mem_ctx, NULL),
> + field_name);
> + e1 = new(mem_ctx) ir_dereference_record(op1->clone(mem_ctx, NULL),
> + field_name);
This also looks wrong. The later lines should match with the (.
> + result = do_comparison(mem_ctx, operation, e0, e1);
> +
> + if (cmp) {
> + cmp = new(mem_ctx) ir_expression(join_op, cmp, result);
> + } else {
> + cmp = result;
> + }
> }
> break;
> }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
