On 12/14/20 9:26 AM, Henri Verbeet wrote:
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.
yyerror() is one example where we need to preserve a location that's not the "current" one, but it actually ends up being most errors.
In the case of the HLSL compiler, except for a scant few errors generated by the lexer, pretty much everything has a location attached that doesn't match the lexer's current location. This is partly because of lookahead (e.g. we want to complain about variable attributes on a function, and have the caret point toward the invalid attribute, but we don't know which one we've parsed yet), partly because bison makes no guarantees about where the lexer actually is when a given rule is parsed, and partly because we report some errors *after* having parsed the whole file [which we do by saving the locations of code that caused us to generate instructions and other IR objects].
In the case of the preprocessor, it's a bit odd, because most errors can be blamed on a malformed or misplaced directive [and so even including the column in the first place doesn't necessarily make a lot of sense]. It'd be arguably possible to always pass the current line and file name, except that it'd actually need to be the previous line and file name, since we've already parsed a newline at that point. But this is also sort of depending on bison internals that I don't think are guaranteed.
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.
Well, I did add preproc_error() as a helper (and there are already similar helpers for the HLSL compiler) so that it wouldn't be too much of a problem. But it is of course nice to not punt the problem, and it's not too hard...
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.