On Tue Apr 9 22:59:29 2024 +0000, Henri Verbeet wrote:
This seems a little fragile.
I don't think it is that fragile, currently we handle directives in 3
points in run_shader_tests():
- The first switch, where we detect that we stopped reading the block
because a new block starts or the file ended.
- The reading of the tags at the beginning of new blocks.
- The reading of individual lines using a second switch.
We don't need to call update_line_number_context() in 1, and we always
have to call it in 2 so we don't have to worry about forgetting to do so we add new functionalities.
It is only in 3 that we have to remember to call
update_line_number_context() for the switch cases when we want to update the line number context for each line instead of presenting it at the beginning of the block, but even if we forget, the error will be reported at the beginning of the block because of 2. Sure, the impact of messing up isn't going to very bad. Fundamentally though, this commit puts the burden of calling update_line_number_context() on the "normal" cases instead of on the exceptional cases.
Could we just track the starting line for things touching
"shader_source" and then setup/restore the test context in e.g. compile_shader()?
Here is a patch for that proposal (how I see it), but I find it more complex.
[0001-tests-shader-runner-Report-compilation-errors-on-the.patch](https://gitlab.winehq.org/wine/vkd3d/uploads/02a5639e5c17c001d23e1d89105c949...)
I think that's closer to what I had in mind. How do you feel about [this patch](/uploads/e4b238bc14f76573c5eea824d660402f/patch.diff)?
While that patch moves the burden to the _exceptional_ cases, with it we have to do many things for them now: - Set `current_source.start_line = runner->current_line` on 2. - Call `update_line_number_context(runner, source->start_line)` before performing the operations. - Call `update_line_number_context(runner, runner->current_line)` after performing the operations. - Call `clear_shader_source(¤t_source)` after the source is stored in the `runner->XX_source` field
I still think that only remembering to call `update_line_number_context()` on _normal_ cases is simpler, and not so hard to forget. When implementing new cases, at least I, take a look to other cases in the same switch for reference.