From patch 1/5:
I prefer avoiding the vkd3d_* prefix on all internal functions, for these reasons. However, I'm open to restoring it.
Note that this can make it harder to find the affected code from backtraces and logs, and in some cases bug tracker searches. I.e., suppose you're getting an error message from an application running in wine mentioning "spirv_compiler_some_function()". Does that come from Mesa, vkd3d, wined3d, or perhaps SPIRV-Tools?
That strikes me as a reasonable concern, but I think that's something we can better resolve by simply always outputting "vkd3d" in the debug functions. At least, I find that preferable to putting a vkd3d prefix on every internal function.
From patch 2/5:
vkd3d-shader/dxbc: Use the VKD3D_DXBC_MAX_SOURCE_COUNT macro where possible.
The macro is unfortunately named, but this touches the shader model 4 (TPF) parser, for which we use the "vkd3d-shader/sm4" prefix. Similarly, if we're going to rename this in 3/5, ideally that would be to something like VKD3D_SM4_MAX_SOURCE_COUNT.
Sure.
If we're cleaning things up, I'd argue for moving the SM4 parser out of dxbc.c, into its own file, like sm4.c or perhaps tpf.c.
That seems reasonable.
I'd also argue for merging hlsl_sm4.c and sm4.h into the same file.
This may also be reasonable, although I think it may depend on more prototyping being done first...
From patch 4/5:
- ins->src_count = strlen(opcode_info->src_info); + ins->src_count = strnlen(opcode_info->src_info, DXBC_MAX_SOURCE_COUNT);As it happens, I have a bit of a dislike for the strn*() functions. You'll find very few of them in vkd3d or wined3d...
I don't understand this one. I suspect the prescription is "use memchr() instead", but that's less convenient. I can see how strncat() and strncpy() are fundamentally broken, but I don't see why the same applies to strnlen(), at least in this case. (Granted, this may be the only usage where strnlen() ever makes sense, but...)
I suppose the other option is to make the array of size DXBC_MAX_SOURCE_COUNT + 1, but the problem here is that the compiler won't catch it for us if we overflow that array by one.
From patch 5/5:
This cuts about 7 kB off of the 64-bit build.
Is that a debug build, or stripped? Avoiding pointers in struct vkd3d_sm4_opcode_info is generally desirable, but I'd expect the "read_opcode_func" field to be the bigger problem with that. Ideally we'd get rid of the linear search as well...
It applies to both stripped and unstripped builds. (Actually, the benefit is more like 12 kB on this machine, not sure what changed.)
I suspect there is a more salient benefit in speed, but I didn't really want to put the effort into measuring that, and the size benefit seemed worthwhile by itself.
- struct vkd3d_shader_dst_param dst_param[2]; + struct vkd3d_shader_dst_param dst_param[DXBC_MAX_DEST_COUNT];
DXBC_MAX_DEST_COUNT is unfortunate. Like above, I'd prefer VKD3D_SM4_ over DXBC_. However, we also use either "src/dst" or "source/destination"; never "dest".
Sure, I'll replace with src/dst.