I'm not quite as convinced it's an improvement, but the broader point is that it's an unrelated change, and should go in its own MR. It happens to touch some lines that the rest of this series also touches, but that can't be the only reason to include something in a series.
Fair point. I will drop the patch then.
I don't hate it, but I think we can do better. In particular, I don't think we need these lookup tables before calling hlsl_sm4_write(), and that means we can just call hlsl_new_sm4_lookup_tables() there, and keep all the lookup table bits local to tpf.c.
I thought on putting the lookup tables in `struct tpf_writer` but then I thought that it may be more futureproof to pass the ctx as argument to all the functions than the writer.
To put this in the broader picture, we'd ultimately like the HLSL compiler to produce a vkd3d_shader_instruction_array structure, and the TPF writer to consume that structure. Ideally the functions writing TPF then wouldn't have direct knowledge about HLSL IR, and wouldn't get passed a hlsl_ctx structure. It'll probably be a while before we get there, but I think we should at least try to avoid exposing TPF details outside tpf.c. Similarly, we probably want to avoid using hlsl_ctx in TPF writer functions that currently don't need it.
I see. Thanks, I appreciate broader pictures!
I will modify the series to do that then.
I don't think "lookup" needs to be a pointer here, right?
That was out of necessity, because `struct vkd3d_sm4_lookup_tables` used structs only defined in tpf.c, so it had to be an incomplete type, so I had to use a pointer.
I guess it will no longer be necessary (together with allocation the lookup tables in the heap) if we put in in the ~~parser~~ writer.
Now that we're no longer accessing register_type_table[] directly, we can make it local to hlsl_new_sm4_lookup_tables().
Okay.
You can't do that. "register_type"/"sm4_type" is read from the bytecode, which is untrusted input. The existing code is perhaps not great in this regard either, but replacing the existing check with an assert makes it worse.
Okay, I suspected such a thing after sending the series. I will make the lookups to return NULL if they are outside the table.