[PATCH vkd3d 02/12] vkd3d-shader/hlsl: Pass field to add_record_load() via index.

[Francisco Casas]

Signed-off-by: Francisco Casas 
---
 libs/vkd3d-shader/hlsl.y | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index f293b82e..8fd18868 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -669,10 +669,14 @@ static struct hlsl_ir_load *add_load(struct hlsl_ctx *ctx, struct list *instrs,
 }

static bool add_record_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *record,
-        const struct hlsl_struct_field *field, const struct vkd3d_shader_location loc)
+        unsigned int idx, const struct vkd3d_shader_location loc)
 {
+    const struct hlsl_struct_field *field;
     struct hlsl_ir_constant *c;

+    assert(idx < record->data_type->e.record.field_count);
+    field = &record->data_type->e.record.fields[idx];
+
     if (!(c = hlsl_new_uint_constant(ctx, field->reg_offset, &loc)))
         return false;
     list_add_tail(instrs, &c->node.entry);
@@ -806,14 +810,18 @@ static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hls
 }

static const struct hlsl_struct_field *get_struct_field(const struct hlsl_struct_field *fields,
-        size_t count, const char *name)
+        size_t count, const char *name, unsigned int *field_indx)
 {
     size_t i;

for (i = 0; i < count; ++i)
     {
         if (!strcmp(fields[i].name, name))
+        {
+            if (field_indx)
+                *field_indx = i;
             return &fields[i];
+        }
     }
     return NULL;
 }
@@ -3050,7 +3058,7 @@ fields_list:
                 const struct hlsl_struct_field *field = &$2.fields[i];
                 const struct hlsl_struct_field *existing;

-                if ((existing = get_struct_field($1.fields, $1.count, field->name)))
+                if ((existing = get_struct_field($1.fields, $1.count, field->name, NULL)))
                 {
                     hlsl_error(ctx, &field->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED,
                             "Field "%s" is already defined.", field->name);
@@ -3947,15 +3955,15 @@ postfix_expr:
             if (node->data_type->type == HLSL_CLASS_STRUCT)
             {
                 struct hlsl_type *type = node->data_type;
-                const struct hlsl_struct_field *field;
+                unsigned int field_idx;

-                if (!(field = get_struct_field(type->e.record.fields, type->e.record.field_count, $3)))
+                if (!get_struct_field(type->e.record.fields, type->e.record.field_count, $3, &field_idx))
                 {
                     hlsl_error(ctx, &@3, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Field "%s" is not defined.", $3);
                     YYABORT;
                 }

-                if (!add_record_load(ctx, $1, node, field, @2))
+                if (!add_record_load(ctx, $1, node, field_idx, @2))
                     YYABORT;
                 $$ = $1;
             }

[Giovanni Mascellani]

Hi,

Il 01/07/22 23:24, Francisco Casas ha scritto:
> static const struct hlsl_struct_field *get_struct_field(const struct hlsl_struct_field *fields,
> -
>    size_t count, const char *name)
> -
>    size_t count, const char *name, unsigned int *field_indx)
>
> {
>     size_t i;
> for (i = 0; i < count; ++i)
> {
>     if (!strcmp(fields[i].name, name))
> -
>    {
> -
>        if (field_indx)
> -
>            *field_indx = i;
>         return &fields[i];
> -
>    }
> }
> return NULL;
>
> }

It's not a big deal, but this interface looks more complicated than it 
should for such a simple thing. Maybe we could just return the field 
index and the caller can dereference the array if needed?

Giovanni.

[Henri Verbeet]

On Tue, 5 Jul 2022 at 11:26, Giovanni Mascellani
 wrote:
> It's not a big deal, but this interface looks more complicated than it
> should for such a simple thing. Maybe we could just return the field
> index and the caller can dereference the array if needed?

Right, you should only ever need one or the other. (I.e., &fields[i] -
fields == i)

[Francisco Casas]

On 05-07-22 06:38, Henri Verbeet wrote:
> On Tue, 5 Jul 2022 at 11:26, Giovanni Mascellani
>  wrote:
> > It's not a big deal, but this interface looks more complicated than it
> > should for such a simple thing. Maybe we could just return the field
> > index and the caller can dereference the array if needed?
>
> Right, you should only ever need one or the other. (I.e., &fields[i] -
> fields == i)

I can do it but I would have to return a magic value (I am thinking on 
-1) when the field is not found, given the current uses of 
get_struct_field().

Alternatively, I can change the return type to bool and return false 
when the field is not found. This seems better.

I can also split the function into a get_struct_field() and a 
has_struct_field(), but I don't like the idea.

[Giovanni Mascellani]

Il 05/07/22 17:15, Francisco Casas ha scritto:
> I can do it but I would have to return a magic value (I am thinking on 
> -1) when the field is not found, given the current uses of 
> get_struct_field().

As Henri said, you can return a pointer (which gives you the NULL value 
to signal a missing field) and then use pointer arithmetic to work out 
the index.

Returning -1 is also fine for me, I don't know if that's acceptable in 
vkd3d.

Personally I would try to avoid pointers-as-return-values when possible.

Giovanni.