Re: [1/8] d3d8: Use D3DPOOL <-> WINED3DPOOL enum conversion functions instead of implicit conversions (clang)
Frédéric Delanoy <frederic.delanoy(a)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. -- Alexandre Julliard julliard(a)winehq.org
-----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.
-----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJN5LsaAAoJEN0/YqbEcdMwMBYQAJfhdtFoBi3dq0nDvK5PwdN3 V5elaAUp3KTwugfmhy9TL1VhPY9QyOQ0x0/dXbd9dqH1cNn54IvU8rl0tcRqlW5B pmeD2fh+eywSGaw2UBhQJjQTHOcRwXo8CJTb/6Z5D10Gt0YNcUAyaQOgQkThiIbI ttItwkM6N9HF+lmWSQ66ue6jNe+8lf0b9pC+r5I8HLi/fUL0L4PglgKTBP/rs8kq YgQmiSRuJ+tP4bzlr8XyuoZEynnL2QJG/60q3M6qC9iA+BR1tHHJsAXQflErICZr v/MrQwbiBjAASXMdmRqxAF9hyA6Qv9cRKbOtEIeI8GPoOg2/V3QbNRiI2GcLMz6s O8zPUxzwbGfuJDQKh04cAsMAIw32JNh1IAPz+GcS9oXzVvCuH+PlfQiLGz+Lxukf jwup8N/3UojvVHU6C8PUDgYV6+eOm0Pbr9j2BUVIj+nliycQlsnrj6P/zthI+r9m tgh152AV8RN52zBZTQlf258KqV1Thw8LnCAkA+a4mG1Bz6gGh4u/o9UDFY8rbENi tSEugEl0w6OecHJBOHTpOPenHHyE3KKCwNZ3bsC/TE4AnuXUMGLoPw88dWCfnrS9 gtvX2pyIpdbjuktAaAPoM15QNZvV16+hmstGXNsrEPgi/p+U6JHw5GvAPtgccPDD iBVo1M4T31Jph8Q3VcGj =snIO -----END PGP SIGNATURE-----
Stefan Dösinger <stefandoesinger(a)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. -- Alexandre Julliard julliard(a)winehq.org
-----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. -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJN5MOyAAoJEN0/YqbEcdMwmFkP/AtNv+3BKIuaY08uDmrpcSI0 qvudinT7Yw6bA0B7TKRIbmgfGVFHc+u+GGfl6bk035LXDL0DBmeo2eYRsS3V3pSq 8lDd/T2g1MDCCFjUx9qoQJeHNGb9qN5gU+ztQeFLynsxvdvCmHNT+ztPejLUSLfe mDIKFZns1/Y6YtyqEbpmCTLjy/WadTeLCj0PfhNERCIujyyDxxdudcqiM2uTCmwe 01wSjISTXcwl3vUdevYOZ2phHHOkPrAQ5mhz0Y1vyTqqg/1sySuKfK4EyZvAiguC 9CSH4nDoiU8OriXFvfCQxHwKJ3dxVfP+EOh3BOXbaB/Nc90+5zKzQYPJzzLEp7uT bX0WkKJHr8R/thlQ/q3zYU2QvTjSVfk+eqdjnKZw5jziWdeM66SdBLeahsubyqOv poIepool5HqPt2oqFvzTyrvJzX730isSu1cy2vU8DmxLGh9H8zrExbPm3Mj16jcO lwapsIxHOagwhVvO1w8TzXGS5KuuRC6TQMaujEVJihdidf0F478YuSw2dpQynOra jaxHwjU+Hy9Tf0HWLr9VefV/mTvUDLe3e07c1QxcU6ImEwQx8QHgViN2nbgCshco WfJefQTTFHg73cN4F9gkXjfIM1Yl3SNcz387spL2GXb3OB3fvAZeinzs9rlUi3b/ sUBcqL50lIuwUgUKOzrg =6HyE -----END PGP SIGNATURE-----
Stefan Dösinger <stefandoesinger(a)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. -- Alexandre Julliard julliard(a)winehq.org
-----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). -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJN5QCKAAoJEN0/YqbEcdMwELoQAIGzt1Pz8xDjTl0872+Eo4ul eTTopbvjUf1b44p0SSQBk8ypwP8FXn1K72wQxk0XcQBr5LOsX8T3i2gDKOwjfMtS YQ4OoEep0+LYdz69bsPydnFE3PNnT8Uhj8v4Di2udfnv5pXp16Ay9y45XfyzFbCY C5Zho8U4V43yWXpyiNF7o1wbSg82Bvkr9YsZJ9e2r1mTmHMKyJfdsjc5kUyDR9el u51Zad53xv99rLa8qmh6lXerbHeGXGTuBv5hMlv0eCWHQyrLvPh0aUvboke83UyE QbM9WME91WhcPlm1VEvA6grP+XZrb20Ib6XINSk804hMZNZlH3r20+kv/3owd2RN IqUxv5KgqR7st1GCOdhzeHo5CorR0JwmNIu68bOO+vJ5fS+b56MORNJ9AO/T+tKN e5pIoUdA2frwqj+/O/pZTpPTvLm+1GjR7/Zp7Dbc4hzqkxmqBRDFsyooBP9ijKzm 9odh6xTI+Al3rUaGxs4mpzAqnWEHGU6IkWPRHcJ3eNtAPRdrahjRCffLH3mT98W0 KsW8CreW2dUBwV/Qreio5jWoSApoEm/k2C/GzaNvhXWCzs0oKcLx7gO6T6uwGowD /76Me1KxRygrmntZprWN5iDEs6+PAsF3Cc6CI1MeNj3a3MYxRqLwj+yuGhjatOgu GuLlEila6eHSAAMoHMx0 =w8v0 -----END PGP SIGNATURE-----
On 31 May 2011 12:50, Alexandre Julliard <julliard(a)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(a)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
participants (4)
-
Alexandre Julliard -
Frédéric Delanoy -
Henri Verbeet -
Stefan Dösinger