2016-03-17 12:59 GMT+01:00 Paul Gofman gofmanp@gmail.com:
Nice job. I have a few nitpicks still...
+static const D3DXVECTOR4 test_effect_preshader_fconstsv[] = +{
- {11.0f, 12.0f, 13.0f, 0.0f},
- {21.0f, 22.0f, 23.0f, 0.0f},
- {31.0f, 32.0f, 33.0f, 0.0f},
- {41.0f, 42.0f, 43.0f, 0.0f},
- {11.0f, 21.0f, 31.0f, 0.0f},
- {12.0f, 22.0f, 32.0f, 0.0f},
- {13.0f, 23.0f, 33.0f, 0.0f},
- {14.0f, 24.0f, 34.0f, 0.0f},
- {11.0f, 12.0f, 13.0f, 14.0f},
- {21.0f, 22.0f, 23.0f, 24.0f},
- {31.0f, 32.0f, 33.0f, 34.0f},
- {11.0f, 21.0f, 31.0f, 41.0f},
- {12.0f, 22.0f, 32.0f, 42.0f},
- {13.0f, 23.0f, 33.0f, 43.0f},
- {4.0f, 5.0f, 6.0f, 7.0f}
+};
+static const D3DXVECTOR4 test_effect_preshader_fconstsp[] = +{
- {11.0f, 21.0f, 0.0f, 0.0f},
- {12.0f, 22.0f, 0.0f, 0.0f},
- {13.0f, 23.0f, 0.0f, 0.0f},
- {11.0f, 12.0f, 0.0f, 0.0f},
- {21.0f, 22.0f, 0.0f, 0.0f},
- {31.0f, 32.0f, 0.0f, 0.0f},
- {11.0f, 12.0f, 0.0f, 0.0f},
- {21.0f, 22.0f, 0.0f, 0.0f},
- {11.0f, 21.0f, 0.0f, 0.0f},
- {12.0f, 22.0f, 0.0f, 0.0f},
- {11.0f, 12.0f, 13.0f, 0.0f},
- {21.0f, 22.0f, 23.0f, 0.0f},
- {11.0f, 21.0f, 31.0f, 0.0f},
- {12.0f, 22.0f, 32.0f, 0.0f}
+};
+static const BOOL test_effect_preshader_bconsts[] = +{
- TRUE, FALSE, TRUE, FALSE, TRUE, FALSE
+};
+static const struct +{
- const char *comment;
- unsigned int todo[4];
- unsigned int result[4];
+} +test_effect_preshader_op_results[] = +{
- {"1 / op", {1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbee8ba2e, 0x00200000}},
- {"rsq", {1, 1, 3, 3}, {0x7f800000, 0x7f800000, 0x3f2c985d, 0x1f800000}},
- {"mul", {1, 1, 1, 1}, {0x00000000, 0x80000000, 0x40d33334, 0x7f800000}},
- {"add", {0, 1, 1, 1}, {0x3f800000, 0x40000000, 0xc0a66666, 0x7f7fffff}},
- {"lt", {0, 0, 1, 0}, {0x3f800000, 0x3f800000, 0x00000000, 0x00000000}},
- {"ge", {1, 1, 0, 1}, {0x00000000, 0x00000000, 0x3f800000, 0x3f800000}},
- {"neg", {1, 1, 1, 1}, {0x80000000, 0x00000000, 0x400ccccd, 0xff7fffff}},
- {"rcp", {1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbee8ba2e, 0x00200000}},
- {"frac", {0, 0, 1, 0}, {0x00000000, 0x00000000, 0x3f4ccccc, 0x00000000}},
- {"min", {0, 1, 1, 1}, {0x00000000, 0x80000000, 0xc0400000, 0x40800000}},
- {"max", {1, 1, 1, 1}, {0x3f800000, 0x40000000, 0xc00ccccd, 0x7f7fffff}},
- {"sin", {0, 1, 1, 3}, {0x00000000, 0x80000000, 0xbf4ef99e, 0xbf0599b3}},
- {"cos", {1, 1, 1, 3}, {0x3f800000, 0x3f800000, 0xbf16a803, 0x3f5a5f96}},
- {"den mul",{1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbb94f209, 0x000051ec}},
- {"dot", {0, 1, 1, 0}, {0x00000000, 0x7f800000, 0x41f00000, 0x00000000}},
- {"prec", {1, 1, 2, 0}, {0x2b8cbccc, 0x2c0cbccc, 0x00000000, 0x00000000}}
+};
+#define TEST_EFFECT_PRES_NFLOATV ARRAY_SIZE(test_effect_preshader_fconstsv) +#define TEST_EFFECT_PRES_NFLOATP ARRAY_SIZE(test_effect_preshader_fconstsp) +#define TEST_EFFECT_PRES_NFLOATMAX (TEST_EFFECT_PRES_NFLOATV > TEST_EFFECT_PRES_NFLOATP ? \
TEST_EFFECT_PRES_NFLOATV : TEST_EFFECT_PRES_NFLOATP)+#define TEST_EFFECT_PRES_NBOOL ARRAY_SIZE(test_effect_preshader_bconsts) +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results)
Those arrays above should probably be inside test_effect_preshader(), unless you plan to add other functions needing them.
- for ( ; i < 16; ++i)
Those spaces in place of the first "for" expression look weird. Just leave that empty i.e.
for (; i < 16; ++i)
- for (i = 0; i < TEST_EFFECT_PRES_NOPTESTS; ++i)
- {
unsigned int *v;hr = IDirect3DDevice9_GetLight(device, i % 8, &light);v = i < 8 ? (unsigned int *)&light.Diffuse : (unsigned int *)&light.Ambient;ok(hr == D3D_OK, "Got result %#x.\n", hr);for (j = 0; j < 4; ++j)todo_wine_if(test_effect_preshader_op_results[i].todo[j] & 1)ok(v[j] == test_effect_preshader_op_results[i].result[j]|| ((test_effect_preshader_op_results[i].todo[j] & 2)&& broken(v[j] != test_effect_preshader_op_results[i].result[j])),"Operation %s, component %u, expected %#x, got %#x (%g).\n",test_effect_preshader_op_results[i].comment, j,test_effect_preshader_op_results[i].result[j], v[j], ((float *)v)[j]);
I don't like that broken(), it isn't checking much (i.e. the condition is true for almost any value) and overloading the "todo" field for that doesn't seem too clean.
Maybe something like this for the test results table would help:
static const struct { const char *name; unsigned int expected[4]; BOOL todo[4]; unsigned int expected_broken[4]; }
I think all the broken results are != 0 so expected_broken also works as a flag. If that's not the case you can add explicit "BOOL broken[4]" flags. You can then leave out the todo and expected_broken fields for the table entries which don't require them, since they end up zero-initialized.
Maybe also add a small comment for each broken value, if it makes sense?
- todo_wine ok(!memcmp(byte_code,
&test_effect_preshader_effect_blob[TEST_EFFECT_PRESHADER_VSHADER_POS +1 * TEST_EFFECT_PRESHADER_VSHADER_LEN], byte_code_size),
You can leave out the "1 *", it's pretty clear either way.
On 03/19/2016 12:52 AM, Matteo Bruni wrote:
2016-03-17 12:59 GMT+01:00 Paul Gofman gofmanp@gmail.com:
I don't like that broken(), it isn't checking much (i.e. the condition is true for almost any value) and overloading the "todo" field for that doesn't seem too clean.
Maybe something like this for the test results table would help:
static const struct { const char *name; unsigned int expected[4]; BOOL todo[4]; unsigned int expected_broken[4]; }
I think all the broken results are != 0 so expected_broken also works as a flag. If that's not the case you can add explicit "BOOL broken[4]" flags. You can then leave out the todo and expected_broken fields for the table entries which don't require them, since they end up zero-initialized.
Maybe then I would rather get rid of "broken" macro at all and rather check different values for 32 bit and 64 bit arch, using expected_64bit instead of expected_broken, which will be filled up only where are the differences? Then there will be a bit of Wine todo's left in the end of the series (4 for win32 and 1 for win64), which are actually there but now not explicitly marked as todo (test results have the value matching wine but "broken" on either win32 or win64).
Or even simpler, I can just #ifdef three lines in result array, and then get rid of "broken()", and get "todo" field as simple BOOL; all this without adding an extra field for 64 bit or broken results.
On 03/19/2016 02:02 AM, Paul Gofman wrote:
On 03/19/2016 12:52 AM, Matteo Bruni wrote:
2016-03-17 12:59 GMT+01:00 Paul Gofman gofmanp@gmail.com:
I don't like that broken(), it isn't checking much (i.e. the condition is true for almost any value) and overloading the "todo" field for that doesn't seem too clean.
Maybe something like this for the test results table would help:
static const struct { const char *name; unsigned int expected[4]; BOOL todo[4]; unsigned int expected_broken[4]; }
I think all the broken results are != 0 so expected_broken also works as a flag. If that's not the case you can add explicit "BOOL broken[4]" flags. You can then leave out the todo and expected_broken fields for the table entries which don't require them, since they end up zero-initialized.
Maybe then I would rather get rid of "broken" macro at all and rather check different values for 32 bit and 64 bit arch, using expected_64bit instead of expected_broken, which will be filled up only where are the differences? Then there will be a bit of Wine todo's left in the end of the series (4 for win32 and 1 for win64), which are actually there but now not explicitly marked as todo (test results have the value matching wine but "broken" on either win32 or win64).
-
Matteo Bruni -
Paul Gofman