Hello,
April 28, 2022 10:49 AM, "Giovanni Mascellani" gmascellani@codeweavers.com wrote:
Hi,
I think the patch generally looks good, but there are a few details to fix.
Il 28/04/22 15:32, Giovanni Mascellani ha scritto:
@@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *ba
list_add_tail(&ctx->types, &type->entry);
- if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
- TRACE("Implicit element count.\n");
I don't understand what this should be useful for, is it perhaps a leftover of development?
Yes, probably a leftover. I think I was worried adding the special case of implicit arrays could generate bugs in other parts of the code if they didn't consider it. But that shouldn't be the case with this implementation since this special case is not present after the initialization.
return type; }
@@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type)
} if (type->type == HLSL_CLASS_ARRAY) {
- assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT);
return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count; } if (type->type != HLSL_CLASS_STRUCT) @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru }
for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type)
- vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
- {
- if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
- vkd3d_string_buffer_printf(string, "[<undefined>]");
- else
- vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
- }
I guess elements_count should never be IMPLICIT here, should it? As soon as the hunk in declare_vars() is done, all the counts are known.
Yes, but I think we better cover all cases in a function we may be using for debugging purposes.
At any rate, if it can happen that the IMPLICIT branch can be taken here, then I'd just use "[]" instead of "[<undefined>]", which is the standard C way.
I think I opted for this verbose alternative since finding an implicit size array in the wild would be an oddity worth emphasizing. But maybe I was paranoid. I will change it to "[]".
return string; }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 28b2ff1b..94597999 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -227,6 +227,8 @@ struct hlsl_src
#define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR) +#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff
struct hlsl_reg_reservation { char type; diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
type = basic_type;
for (i = 0; i < v->arrays.count; ++i)
- {
- if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
- {
- unsigned int size = initializer_size(&v->initializer);
- unsigned int elem_components = hlsl_type_component_count(type);
- assert(v->initializer.args_count);
Maybe it's just me, but here I would also assert that i == v->arrays.count - 1. This should already be forced by the parser, but it's a useful code documentation anyway.
Sounds good.
- v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
Why "+ elem_components - 1", given that immediately below we error out if size is not a multiple of elem_components?
Also, notice that elem_components could be zero. You should handle that gracefully.
These are good points, I will update the patch.
- if (size % elem_components != 0)
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
- "Cannot initialize implicit array with %u components, expected a multiple of %u.",
- size, elem_components);
- free_parse_initializer(&v->initializer);
- v->initializer.args_count = 0;
- }
- }
type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
- }
vkd3d_free(v->arrays.sizes);
Thanks, Giovanni.