April 9, 2024
5:59 p.m.
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(): > > > > 1. The first switch, where we detect that we stopped reading the block > because a new block starts or the file ended. > > 2. The reading of the tags at the beginning of new blocks. > > 3. 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/02a5639e5c17c001d23e1d89105c949b/0001-tests-shader-runner-Report-compilation-errors-on-the.patch) > 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/761#note_67443