On Sat, 12 Dec 2020 at 01:52, Zebediah Figura (she/her) zfigura@codeweavers.com wrote:
On 12/11/20 12:11 PM, Henri Verbeet wrote:
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura zfigura@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.