From: Giovanni Mascellani Subject: Re: [PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan(). Message-Id: <0b9f87d9-6805-baab-c7a1-91d2adf6fcfe@codeweavers.com> Date: Fri, 7 Jan 2022 11:44:13 +0100 In-Reply-To: References: <20211228222020.13814-1-zfigura@codeweavers.com> <20211228222020.13814-3-zfigura@codeweavers.com> Hi, On 06/01/22 11:15, Henri Verbeet wrote: > On Tue, 28 Dec 2021 at 23:20, Zebediah Figura wrote: >> Besides being the first patch implementing reflection, this patch introduces an >> API problem with regard to D3DReflect() that will affect many different fields. >> Namely, if the SM4 version token contains a nonsensical version D3DReflect() >> will happily return it verbatim. As I see it our options are: >> >> (1) return the token verbatim anyway instead of using struct >> vkd3d_shader_version, >> >> (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge >> from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan() >> there either] >> >> (3) return the token verbatim in a separate field (and in a separate chained >> structure?) >> >> (4) punt, and use one of the above solutions if we ever do come across a shader >> that contains insane data. >> > I'd be inclined to go with option 3 here. I don't think I have a complete understanding of the matter, but based on what I gather I'd say that it makes sense to report both interpreted and uninterpreted data, so that the caller can choose what they believe it's best for them. I guess it's option 3 here. No strong opinion of whether to using the same or another chained structure (weak opinion is to use the same). >> * Instruction counts (and I think everything else in the STAT chunk) are always >> taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will >> return zero, even though it could just count them. Should we preserve that >> behaviour in libvkd3d-shader? in libvkd3d-utils? >> > If we're going with something like > vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes. In the same philosophy, I'd make both the reported and recomputed data available, so that the caller can choose. If it is expensive to recompute, I'd gate that with a flag. Giovanni.