"Christopher" <raccoonone(a)procyongames.com> wrote:
+static void test_LoadStringW(void) +{ + HINSTANCE hInst = GetModuleHandle(NULL); + WCHAR copiedstring[128], returnedstring[128], *resourcepointer = NULL; + int strlen, strlen2; + + /* Check that the string which is returned by LoadStringW matches + the string at the pointer returned by Load StringW when called with buflen = 0 */ + strlen = LoadStringW(hInst, 2, (WCHAR *) &resourcepointer, 0); /* get pointer to resource. */ + ok(strlen > 0, "LoadStringW failed to get pointer to resource 2, ret %d, err %d \n", strlen, GetLastError()); + + strlen2 = LoadStringW(hInst, 2, returnedstring, sizeof(returnedstring) /sizeof(WCHAR)); + ok(strlen == strlen2, "LoadStringW returned different values dependent on buflen. ret1 %d, ret2 %d \n", + strlen, strlen2); + ok(strlen2 > 0, "LoadStringW failed to load resource 2, ret %d, err %d \n", strlen2, GetLastError()); + + if(resourcepointer != NULL) + { + memcpy(copiedstring, resourcepointer, strlen * sizeof(WCHAR)); + copiedstring[strlen] = '\0'; + } + /* check that strings match */ + ok(!memcmp(copiedstring, returnedstring, (strlen + 1)*sizeof(WCHAR)), + "LoadStringW returned a string that does not match the string pointed to by the pointer it returned. \ + returnedstring = %ls, copiedstring = %ls", returnedstring, copiedstring); +}
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed. Also, if the test depends on a later patch to not fail, the test should be included in the patch. -- Dmitry.
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0. In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix. -- Dmitry.
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
Why do I need to test LoadStringA? My patch only deals with LoadStringW. Christopher
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
On Jan 21, 2008 2:14 PM, Christopher <raccoonone(a)procyongames.com> wrote:
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
That's why you need to add tests for LoadStringA to Wine's test suite. -- James Hawkins
James Hawkins wrote:
On Jan 21, 2008 2:14 PM, Christopher <raccoonone(a)procyongames.com> wrote:
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
That's why you need to add tests for LoadStringA to Wine's test suite.
How can I test for an access violation, won't that crash the test suite? Also, what's the guideline for what functions I need to write tests for when I send in a patch? I still don't understand why I need to write a test for LoadStringA since it has no dependence on LoadStringW (the function I'm patching).
On Jan 21, 2008 2:47 PM, Christopher <raccoonone(a)procyongames.com> wrote:
James Hawkins wrote:
On Jan 21, 2008 2:14 PM, Christopher <raccoonone(a)procyongames.com> wrote:
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
That's why you need to add tests for LoadStringA to Wine's test suite.
How can I test for an access violation, won't that crash the test suite? Also, what's the guideline for what functions I need to write tests for when I send in a patch? I still don't understand why I need to write a test for LoadStringA since it has no dependence on LoadStringW (the function I'm patching).
You could start by reading the code. LoadStringA is a wrapper around LoadStringW. -- James Hawkins
James Hawkins wrote:
On Jan 21, 2008 2:47 PM, Christopher <raccoonone(a)procyongames.com> wrote:
James Hawkins wrote:
On Jan 21, 2008 2:14 PM, Christopher <raccoonone(a)procyongames.com> wrote:
Dmitry Timoshkov wrote:
"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
In addition, as I already pointed out you need to inspect Wine source and fix the places which will be broken by your fix.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
That's why you need to add tests for LoadStringA to Wine's test suite.
How can I test for an access violation, won't that crash the test suite? Also, what's the guideline for what functions I need to write tests for when I send in a patch? I still don't understand why I need to write a test for LoadStringA since it has no dependence on LoadStringW (the function I'm patching).
You could start by reading the code. LoadStringA is a wrapper around LoadStringW.
Thank you. I see that I did not read that function carefully. I think this problem is too difficult for me now, so I will submit my conformance test as a to-do
* James Hawkins wrote:
* On Jan 21, 2008 2:14 PM, Christopher wrote:
* Dmitry Timoshkov wrote:
Also, you need to test LoadStringA, to see if it behaves similarly. It would be also interesting to test LoadStringA/W with both buffer and buffer length set to 0.
I tested LoadStringA under Windows XP, and calling it with buflen == 0 does not return a pointer to the resource. In fact LoadStringA seems to behave fairly differently from LoadStringW: in that calling with buffer == NULL causes an access violation instead of just returning 0.
That's why you need to add tests for LoadStringA to Wine's test suite.
No, he did not. The word "need" wasn't appropriate here. Christopher wasn't going to change LoadStringA. If changes of LoadStringW breaks LoadStringA then tests would show regression. If tests are too weak, then only patch commiter or previous patchers of the LoadStringA are to blame, not the casual contributor who enhances B. And if this is some new official rule for code, then I don't find it on the site:www.winehq.org . Of course, I don't protest at the idea (as I would do the same as Dmitry wrote), but I protest against usage of this particular word -- it's too strong here. Please, be more accurate with it. P.S.: James, you probably should start learning to cut unnecessary blocks of quoted text (esp. empty lines) and doing some block justifying in your replies using your MUA some day. Otherwise it gets too hard to read a discussion.
"Saulius Krasuckas" <saulius2(a)ar.fi.lt> wrote:
No, he did not. The word "need" wasn't appropriate here. Christopher wasn't going to change LoadStringA. If changes of LoadStringW breaks LoadStringA then tests would show regression. If tests are too weak, then only patch commiter or previous patchers of the LoadStringA are to blame, not the casual contributor who enhances B.
It should be obvious that if a patch introduces a regression (which is the case) then this patch can't be aplied. If the regression can be identified by someone able to read the code and not by a regression test that doesn't matter.
P.S.: James, you probably should start learning to cut unnecessary blocks of quoted text (esp. empty lines) and doing some block justifying in your replies using your MUA some day. Otherwise it gets too hard to read a discussion.
James left the useful context which is especially helpful for you to reply in 2 weeks without need to reread the whole thread. -- Dmitry.
* On Fri, 4 Apr 2008, Dmitry Timoshkov wrote:
If the regression can be identified by someone able to read the code and not by a regression test that doesn't matter.
Could you show me appropriate code lines of LoadStringA and a logic to follow, please? I am schocked. Dmitry, for me your statement is true only in the case of reading original code of appropriate DLL from MS :(
"Saulius Krasuckas" <saulius2(a)ar.fi.lt> wrote:
If the regression can be identified by someone able to read the code and not by a regression test that doesn't matter.
Could you show me appropriate code lines of LoadStringA and a logic to follow, please? I am schocked.
Dmitry, for me your statement is true only in the case of reading original code of appropriate DLL from MS :(
I suspect that you will be shocked even more if someone would say that something is really wrong out there if there is a need to explain what this list is about, how the patch review process is going, and which code is reviewed in the process. -- Dmitry.
* On Mon, 7 Apr 2008, Dmitry Timoshkov wrote:
* "Saulius Krasuckas" <saulius2(a)ar.fi.lt> wrote:
If the regression can be identified by someone able to read the code and not by a regression test that doesn't matter.
Could you show me appropriate code lines of LoadStringA and a logic to follow, please? I am schocked.
Dmitry, for me your statement is true only in the case of reading original code of appropriate DLL from MS :(
I suspect that you will be shocked even more if someone would say that something is really wrong out there if there is a need to explain what this list is about,
And I am not asking for this. Plus, You still have not answered my question: how regression in question can be identified only by reading Wine code (at least in case of LoadStringA).
how the patch review process is going, and which code is reviewed in the process.
So do you say Wine HQs aren't willing to attract more casual contributors to Wine code via wine-devel@ anymore? :(
"Saulius Krasuckas" <saulius2(a)ar.fi.lt> wrote:
And I am not asking for this. Plus, You still have not answered my question: how regression in question can be identified only by reading Wine code (at least in case of LoadStringA).
I have no idea how the word 'only' has appeared in the sentence above, and why it's not clear to you that reading Wine code is one of possible ways to understand what the patchs does, and see possible improvements or regressions caused by a patch. Or you know development methods which don't require reading and understanding the code you are going to modify?
how the patch review process is going, and which code is reviewed in the process.
So do you say Wine HQs aren't willing to attract more casual contributors to Wine code via wine-devel@ anymore? :(
I don't follow your logic here. -- Dmitry.
* On Wed, 9 Apr 2008, Dmitry Timoshkov wrote:
* "Saulius Krasuckas" <saulius2(a)ar.fi.lt> wrote:
You still have not answered my question: how regression in question can be identified only by reading Wine code (at least in case of LoadStringA).
I have no idea how the word 'only' has appeared in the sentence above,
I am sorry for my hurry, it's really unnecessary here.
and why it's not clear to you that reading Wine code is one of possible ways to understand what the patchs does, and see possible improvements or regressions caused by a patch. Or you know development methods which don't require reading and understanding the code you are going to modify?
Well, what I meant was if you haven't tested LoadStringA in Windows, then reading code won't help you to see a bug in it. Excuse me my English, if earlier msgs of mine didn't sound like that. Such is specifity (peculiarity) of any API translator, no?
I don't follow your logic here.
Well, switching to private comm.
Dmitry Timoshkov wrote:
"Christopher" <raccoonone(a)procyongames.com> wrote:
+static void test_LoadStringW(void) +{ + HINSTANCE hInst = GetModuleHandle(NULL); + WCHAR copiedstring[128], returnedstring[128], *resourcepointer = NULL; + int strlen, strlen2; + + /* Check that the string which is returned by LoadStringW matches + the string at the pointer returned by Load StringW when called with buflen = 0 */ + strlen = LoadStringW(hInst, 2, (WCHAR *) &resourcepointer, 0); /* get pointer to resource. */ + ok(strlen > 0, "LoadStringW failed to get pointer to resource 2, ret %d, err %d \n", strlen, GetLastError()); + + strlen2 = LoadStringW(hInst, 2, returnedstring, sizeof(returnedstring) /sizeof(WCHAR)); + ok(strlen == strlen2, "LoadStringW returned different values dependent on buflen. ret1 %d, ret2 %d \n", + strlen, strlen2); + ok(strlen2 > 0, "LoadStringW failed to load resource 2, ret %d, err %d \n", strlen2, GetLastError()); + + if(resourcepointer != NULL) + { + memcpy(copiedstring, resourcepointer, strlen * sizeof(WCHAR)); + copiedstring[strlen] = '\0'; + } + /* check that strings match */ + ok(!memcmp(copiedstring, returnedstring, (strlen + 1)*sizeof(WCHAR)), + "LoadStringW returned a string that does not match the string pointed to by the pointer it returned. \ + returnedstring = %ls, copiedstring = %ls", returnedstring, copiedstring); +}
It's not clear what this test is supposed to show. If the 1st call to LoadStringW is supposed to set resourcepointer to not NULL, why don't you test it? Then 'if(resourcepointer != NULL)' check and copying to copiedstring are not needed.
Also, if the test depends on a later patch to not fail, the test should be included in the patch.
Even if I check for resourcepointer != NULL in an ok() [which I will add], I still need the if block. String resources are not '\0' terminated, and I wish to compare it to the string that was returned by LoadStringW
participants (4)
-
Christopher -
Dmitry Timoshkov -
James Hawkins -
Saulius Krasuckas