[PATCH vkd3d 4/6] vkd3d-shader: Implement hlsl_error() and hlsl_warning().

[Zebediah Figura (she/her)]

On 2/18/21 5:49 PM, Matteo Bruni wrote:
> On Wed, Feb 17, 2021 at 6:52 AM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>> ---
>>  libs/vkd3d-shader/hlsl.c                 |  18 +-
>>  libs/vkd3d-shader/hlsl.h                 |   8 +-
>>  libs/vkd3d-shader/hlsl.l                 |   3 +-
>>  libs/vkd3d-shader/hlsl.y                 | 204 ++++++++++++-----------
>>  libs/vkd3d-shader/vkd3d_shader_private.h |  18 ++
>>  5 files changed, 147 insertions(+), 104 deletions(-)
> 
> I generally like the changes to the error messages. Regardless, I have
> a couple of comments...
> 
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index c33d97c6..9ffabbcb 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
> 
>> @@ -768,12 +776,13 @@ static bool add_typedef(struct hlsl_ctx *ctx, DWORD modifiers, struct hlsl_type
>>
>>          if ((type->modifiers & HLSL_MODIFIER_COLUMN_MAJOR)
>>                  && (type->modifiers & HLSL_MODIFIER_ROW_MAJOR))
>> -            hlsl_error(ctx, v->loc, "more than one matrix majority keyword");
>> +            hlsl_error(ctx, v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER,
>> +                    "'row_major' and 'column_major' modifiers are mutually exclusive.");
>>
>>          ret = hlsl_scope_add_type(ctx->cur_scope, type);
>>          if (!ret)
>> -            hlsl_error(ctx, v->loc,
>> -                    "redefinition of custom type '%s'", v->name);
>> +            hlsl_error(ctx, v->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED,
>> +                    "Typedef '%s' is already defined.", v->name);
>>          vkd3d_free(v);
>>      }
>>      vkd3d_free(list);
> 
> This made me wonder whether defining a struct also implicitly defines
> a type (i.e. can you reference a struct type by its name without
> 'struct' even when there is no explicit typedef?). A quick test
> suggests that's the case, if that's confirmed we should probably fix
> it at some point.

It does, and it actually already works with our parser. I guess that
makes the message here slightly misleading (maybe "Type '%s'" would be
better?)

> 
>> @@ -1772,15 +1783,11 @@ struct_declaration:
>>              if (!$3)
>>              {
>>                  if (!$2->name)
>> -                {
>> -                    hlsl_error(ctx, @2,
>> -                            "anonymous struct declaration with no variables");
>> -                }
>> +                    hlsl_error(ctx, @2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX,
>> +                            "Struct type definitions cannot be anonymous.");
>>                  if (modifiers)
>> -                {
>> -                    hlsl_error(ctx, @1,
>> -                            "modifier not allowed on struct type declaration");
>> -                }
>> +                    hlsl_error(ctx, @1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER,
>> +                            "Modifiers are not allowed on struct type definitions.");
>>              }
>>
>>              if (!(type = apply_type_modifiers(ctx, $2, &modifiers, @1)))
> 
> Splitting hairs, but I think "declaration" is a bit nicer for both of
> the above (the C spec refers to those as declarations, fwiw). The
> message could be improved to clarify that the declaration being
> anonymous is only a problem if there are no variables being defined at
> the same time (which I assume is what you mean with "Struct type
> definitions"), but I don't have any good practical suggestions.
> 

Fair points, I'll change these.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210220/fc546d57/attachment.sig>