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

[Matteo Bruni]

On Sat, Feb 20, 2021 at 5:11 PM Zebediah Figura (she/her)
<zfigura at codeweavers.com> wrote:
>
> 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?)

Indeed it does work, I must have misread the code. Yes, I think a more
generic "Type" (as you did in v2) is nicer, thanks.