Re: LoadStringW [1/2]

[Dmitry Timoshkov]

"Christopher"  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.

LoadStringW [1/2]

[Dmitry Timoshkov]

"Dmitry Timoshkov"  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.

LoadStringW [1/2]

[Christopher]

Dmitry Timoshkov wrote:
> "Dmitry Timoshkov"  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

LoadStringW [1/2]

[Christopher]

Dmitry Timoshkov wrote:
> "Dmitry Timoshkov"  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.

LoadStringW [1/2]

[James Hawkins]

On Jan 21, 2008 2:14 PM, Christopher  wrote:
> Dmitry Timoshkov wrote:
> > "Dmitry Timoshkov"  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.

LoadStringW [1/2]

[Christopher]

James Hawkins wrote:
> On Jan 21, 2008 2:14 PM, Christopher  wrote:
> > Dmitry Timoshkov wrote:
> > > "Dmitry Timoshkov"  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).

LoadStringW [1/2]

[James Hawkins]

On Jan 21, 2008 2:47 PM, Christopher  wrote:
> James Hawkins wrote:
> > On Jan 21, 2008 2:14 PM, Christopher  wrote:
> > > Dmitry Timoshkov wrote:
> > > > "Dmitry Timoshkov"  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.

LoadStringW [1/2]

[Christopher]

James Hawkins wrote:
> On Jan 21, 2008 2:47 PM, Christopher  wrote:
> > James Hawkins wrote:
> > > On Jan 21, 2008 2:14 PM, Christopher  wrote:
> > > > Dmitry Timoshkov wrote:
> > > > > "Dmitry Timoshkov"  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

LoadStringW [1/2]

[Saulius Krasuckas]

* 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.

LoadStringW [1/2]

[Dmitry Timoshkov]

"Saulius Krasuckas"  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.

LoadStringW [1/2]

[Saulius Krasuckas]

* 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 :(

LoadStringW [1/2]

[Dmitry Timoshkov]

"Saulius Krasuckas"  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.

LoadStringW [1/2]

[Saulius Krasuckas]

* On Mon, 7 Apr 2008, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas"  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? :(

LoadStringW [1/2]

[Dmitry Timoshkov]

"Saulius Krasuckas"  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.

LoadStringW [1/2]

[Saulius Krasuckas]

* On Wed, 9 Apr 2008, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas"  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.

LoadStringW [1/2]

[Christopher]

Dmitry Timoshkov wrote:
> "Christopher"  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