[PATCH v2 vkd3d 1/4] vkd3d-shader: Handle preprocessor parsing errors.

[Henri Verbeet]

On Sat, 12 Dec 2020 at 01:52, Zebediah Figura (she/her)
<zfigura at codeweavers.com> wrote:
>
> On 12/11/20 12:11 PM, Henri Verbeet wrote:
> > On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura at codeweavers.com> wrote:
> >> +struct preproc_location
> >> +{
> >> +    const char *filename;
> >> +    unsigned int first_line, first_column;
> >> +};
> >> +
> >>  struct preproc_ctx
> >>  {
> >>      void *scanner;
> >>
> >> +    struct vkd3d_shader_message_context *message_context;
> >>      struct vkd3d_string_buffer buffer;
> >> +    unsigned int line, column;
> >> +    const char *source_name;
> >> +
> >> +    bool error;
> >>  };
> >>
> > There seems to be some duplication of location information between
> > struct vkd3d_shader_message_context, struct preproc_ctx, and struct
> > preproc_location. Is that intentional? If it is, would it make sense
> > to introduce e.g. a struct vkd3d_shader_source_location instead of
> > struct preproc_location, and then use that structure in both struct
> > vkd3d_shader_message_context and struct preproc_ctx?
>
> Wrt the vkd3d_shader_message_context part—the current structure
> definitely isn't a great fit for the preprocessor or HLSL parser (i.e.
> the parts that use yacc) because the location tracking means that we'll
> be setting a new location on every error call. (And of course most of
> the other parsers we have don't really have a meaningful concept of
> line/column locations...) But I wasn't sure enough to actually try to
> redesign those parts, and it seemed like something that could be
> deferred, so I just left it alone...
>
I probably haven't looked at this series in enough detail, so I may be
missing something obvious, but is the issue that we're getting an
explicit location in yyerror() that may not match the location stored
in struct preproc_ctx? I think it would be fine to pass an explicit
location to vkd3d_shader_error() and similar functions as well, and
moving location tracking out of struct vkd3d_shader_message_context.
That probably doesn't need to delay this series, but if that change is
what we want, it seems easier to make it sooner rather than later.

It does look like we'd want a struct vkd3d_shader_source_location
instead of struct preproc_location in that case.

> Wrt preproc_location vs preproc_ctx—part of the rub here is that {line,
> column} is a bit different from {first_line, first_column}; i.e. if we
> were to add {last_line, last_column}, that wouldn't be a part of
> preproc_ctx.
>
Hmm, right.