Re: [1/8] d3d8: Use D3DPOOL <-> WINED3DPOOL enum conversion functions instead of implicit conversions (clang)
Frédéric Delanoy frederic.delanoy@gmail.com writes:
+static inline D3DPOOL d3dpool_from_wined3dpool(WINED3DPOOL type) +{
- switch (type)
- {
case WINED3DPOOL_DEFAULT: return D3DPOOL_DEFAULT;case WINED3DPOOL_MANAGED: return D3DPOOL_MANAGED;case WINED3DPOOL_SYSTEMMEM: return D3DPOOL_SYSTEMMEM;case WINED3DPOOL_SCRATCH: return D3DPOOL_SCRATCH;case WINED3DPOOL_FORCE_DWORD: return D3DPOOL_FORCE_DWORD;- }
- return (D3DPOOL) type;
+}
+static inline WINED3DPOOL wined3dpool_from_d3dpool(D3DPOOL type) +{
- switch (type)
- {
case D3DPOOL_DEFAULT: return WINED3DPOOL_DEFAULT;case D3DPOOL_MANAGED: return WINED3DPOOL_MANAGED;case D3DPOOL_SYSTEMMEM: return WINED3DPOOL_SYSTEMMEM;case D3DPOOL_SCRATCH: return WINED3DPOOL_SCRATCH;case D3DPOOL_FORCE_DWORD: return WINED3DPOOL_FORCE_DWORD;- }
- return (WINED3DPOOL) type;
+}
I still don't see the point of that kind of switch statement. To be honest I don't see either why we even need different enum types in the first place.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 31.05.2011 um 11:10 schrieb Alexandre Julliard:
I still don't see the point of that kind of switch statement. To be honest I don't see either why we even need different enum types in the first place.
Because for most enums the d3d8 ones differ from the d3d9 ones but have the same name. We cannot include d3d9types.h in d3d8 and ddraw but we still have to talk to wined3d.
Stefan Dösinger stefandoesinger@gmx.at writes:
Am 31.05.2011 um 11:10 schrieb Alexandre Julliard:
I still don't see the point of that kind of switch statement. To be honest I don't see either why we even need different enum types in the first place.
Because for most enums the d3d8 ones differ from the d3d9 ones but have the same name. We cannot include d3d9types.h in d3d8 and ddraw but we still have to talk to wined3d.
Most enums are identical, or just have an extra value or two. I don't see why you can't pass either type to wined3d since they have the same name.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 31.05.2011 um 12:12 schrieb Alexandre Julliard:
Most enums are identical, or just have an extra value or two. I don't see why you can't pass either type to wined3d since they have the same name.
Do you mean the status quo? Or do you mean using the D3DPOOL type in wined3d by including the d3d9 headers(this works in wined3d.dll itself)?
The problem here's ddraw(and d3d10/11), the functionally equivalent types work differently there, they are a set of flags in DDSURFACEDESC2. Yet we can't include d3d8types.h or d3d9types.h in ddraw because there are a handful of types that cause name conflicts. We used to do that a long time ago, but there are only a handful of types that exist in all 3 d3d versions and can be reused. In d3d10 the fixed function pipeline is gone, and so are the types related to it, but they are still needed for the wined3d interface definition.
Stefan Dösinger stefandoesinger@gmx.at writes:
Am 31.05.2011 um 12:12 schrieb Alexandre Julliard:
Most enums are identical, or just have an extra value or two. I don't see why you can't pass either type to wined3d since they have the same name.
Do you mean the status quo? Or do you mean using the D3DPOOL type in wined3d by including the d3d9 headers(this works in wined3d.dll itself)?
I mean using D3DPOOL in wined3d, not by including the d3d9 headers, but by defining it when necessary, i.e. if neither d3d8types.h nor d3d9types.h have been included already.
This way d3d8 uses the d3d8 definition, d3d9 uses the d3d9 definition, and ddraw/d3d10/wined3d internals use the wined3d definition which is probably a superset of the others.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 31.05.2011 um 12:50 schrieb Alexandre Julliard:
I mean using D3DPOOL in wined3d, not by including the d3d9 headers, but by defining it when necessary, i.e. if neither d3d8types.h nor d3d9types.h have been included already.
This would lead to a mixture of WINED3D* and D3D* types. But that could be addressed with a #define WINED3DPOOL D3DPOOL if D3DPOOL is available and declaring the num if D3DPOOL is not available.
There are types like D3DRENDERSTATETYPE that aren't clear subsets/supersets. For example d3d8 has D3DRS_ZBIAS while d3d9 has D3DRS_DEPTHBIAS, and ZBIAS was removed.
I guess that's something we should discuss with Henri once he is back. I don't like the idea just yet because we can't check if the d3d8, d3d9 and wined3d types are the same. With the switch statement without a default case we get a compiler warning if the wined3d type changes and the compiler optimizes the switch statement and no-op function away(At least the last time I checked).
On 31 May 2011 12:50, Alexandre Julliard julliard@winehq.org wrote:
I mean using D3DPOOL in wined3d, not by including the d3d9 headers, but by defining it when necessary, i.e. if neither d3d8types.h nor d3d9types.h have been included already.
This way d3d8 uses the d3d8 definition, d3d9 uses the d3d9 definition, and ddraw/d3d10/wined3d internals use the wined3d definition which is probably a superset of the others.
It's called WINED3DPOOL instead of D3DPOOL since it's a wined3d type, but otherwise I think that's close enough to what we currently have that it's practically the same.
Conceptually I think it makes sense to have a distinction between wined3d types and e.g. ddraw types. For things like D3DPOOL that's not strictly required since it has the same values in all d3d versions where it exists, but e.g. format IDs are incompatible between versions. In that sense calling it WINED3DPOOL instead of D3DPOOL is mostly just a matter of consistency. On the other hand, perhaps the more fundamental point is that I'm not so sure that "superset of all d3d versions" is the correct level of abstraction for wined3d. I suspect we'll see that break down a bit as we implement more of d3d10/11, and perhaps also with some ddraw cleanups. (To pick a random example, consider d3d10/11 sampler states vs d3d8/9 sampler states.)
Note that WINED3DPOOL is a terrible example though, since it should go away in favor of d3d10 style CPU/GPU access masks in the medium to long term anyway. Similarly, it makes much more sense to fix something like IDirect3DTexture8Impl_GetType() by replacing most of the function with just "return D3DRTYPE_TEXTURE;" instead of introducing the d3dresourcetype_from_wined3dresourcetype() function. The existing code should be perfectly valid C, though perhaps not all that pretty in some places. Clang throws some warnings, but well, tough. It's not very high on my list of things I want to spend time thinking about. At this point we probably spent more time talking about it than fixing those enum conversion warnings would ever save anyone. Except for the cases that are obvious to fix / improve, I think it's probably best to just leave it alone. Maybe it will go away if we ignore it.
On Sat, Jun 4, 2011 at 23:50, Henri Verbeet hverbeet@gmail.com wrote:
Conceptually I think it makes sense to have a distinction between wined3d types and e.g. ddraw types. For things like D3DPOOL that's not strictly required since it has the same values in all d3d versions where it exists, but e.g. format IDs are incompatible between versions. In that sense calling it WINED3DPOOL instead of D3DPOOL is mostly just a matter of consistency. On the other hand, perhaps the more fundamental point is that I'm not so sure that "superset of all d3d versions" is the correct level of abstraction for wined3d. I suspect we'll see that break down a bit as we implement more of d3d10/11, and perhaps also with some ddraw cleanups. (To pick a random example, consider d3d10/11 sampler states vs d3d8/9 sampler states.)
Note that WINED3DPOOL is a terrible example though, since it should go away in favor of d3d10 style CPU/GPU access masks in the medium to long term anyway. Similarly, it makes much more sense to fix something like IDirect3DTexture8Impl_GetType() by replacing most of the function with just "return D3DRTYPE_TEXTURE;" instead of introducing the d3dresourcetype_from_wined3dresourcetype() function. The existing code should be perfectly valid C, though perhaps not all that pretty in some places. Clang throws some warnings, but well, tough. It's not very high on my list of things I want to spend time thinking about. At this point we probably spent more time talking about it than fixing those enum conversion warnings would ever save anyone. Except for the cases that are obvious to fix / improve, I think it's probably best to just leave it alone.
OK. In that case I'll just abandon this patch series altogether, since it's probably a waste of effort anyway to try and get something in. However, with or without warnings/clang, passing around non 1-1 mapped enum values is still bad style IMHO, and erroneous values can still be passed unnoticed.
Maybe it will go away if we ignore it.
Definitely when some cleanup is eventually done, but that's an orthogonal argument.
Frédéric Delanoy
-
Alexandre Julliard -
Frédéric Delanoy -
Henri Verbeet -
Stefan Dösinger