Re: [PATCH 01/10] vbscript: Fixed CBool(try 3)

[Jacek Caban]

Hi Shuai,

On 04/21/14 03:45, Shuai Meng wrote:
> -
> - return return_bool(res, val);
> - V_VT(res) = VT_EMPTY;
> - return VariantChangeType(res, arg, VARIANT_LOCALBOOL, VT_BOOL);

You can't assume that res is not NULL. If result of the function is not
used, it will be NULL. A simple test case is like this:

call CBool(0)

Also, are you sure we want VARIANT_LOCALBOOL here? Did you test it
(probably by running tests on localized Windows with translated string
"False")?

Looking at CBool tests:

+Call ok(CBool(Empty) = False, "CBool(Empty) = " & CBool(Empty))
+Call ok(getVT(CBool(Empty)) = "VT_BOOL", "getVT(CBool(Empty)) = " & getVT(CBool(Empty)))
+Call ok(CBool(1) = True, "CBool(1) = " & CBool(1))
+Call ok(getVT(CBool(1)) = "VT_BOOL", "getVT(CBool(1)) = " & getVT(CBool(1)))
+Call ok(CBool(CLng(0)) = False, "CBool(CLng(0)) = " & CBool(CLng(0)))
+Call ok(getVT(CBool(CLng(0))) = "VT_BOOL", "getVT(CBool(CLng(0))) = " & getVT(CBool(CLng(0))))
+Call ok(CBool(CSng(0.5)) = True, "CBool(CSng(0.5)) = " & CBool(CSng(0.5)))
+Call ok(getVT(CBool(CSng(0.5))) = "VT_BOOL", "getVT(CBool(CSng(0.5))) = " & getVT(CBool(CSng(0.5))))
+Call ok(CBool(-0.56) = True, "CBool(-0.56) = " & CBool(-0.56))
+Call ok(getVT(CBool(-0.56)) = "VT_BOOL", "getVT(CBool(-0.56)) = " & getVT(CBool(-0.56)))
+Call ok(CBool(CCur(8.55)) = True, "CBool(CCur(8.55)) = " & CBool(CCur(8.55)))
+Call ok(getVT(CBool(CCur(8.55))) = "VT_BOOL", "getVT(CBool(CCur(8.55))) = " & getVT(CBool(CCur(8.55))))
+Call ok(CBool(CDate(0)) = False, "CBool(CDate(0)) = " & CBool(CDate(0)))
+Call ok(getVT(CBool(CDate(0))) = "VT_BOOL", "getVT(CBool(CDate(0))) = " & getVT(CBool(CDate(0))))
+Call ok(CBool(CDate(3.33)) = True, "CBool(CDate(3.33)) = " & CBool(CDate(3.33)))
+Call ok(getVT(CBool(CDate(3.33))) = "VT_BOOL", "getVT(CBool(CDate(3.33))) = " & getVT(CBool(CDate(3.33))))
+Call ok(CBool("6666.78") = True, "CBool(""6666.78"") = " & CBool("6666.78"))
+Call ok(getVT(CBool("6666.78")) = "VT_BOOL", "getVT(CBool(""6666.78"")) = " & getVT(CBool("6666.78")))
+Call ok(CBool(CByte(-0.1)) = False, "CBool(CByte(-0.1)) = " & CBool(CByte(-0.1)))
+Call ok(getVT(CBool(CByte(-0.1))) = "VT_BOOL", "getVT(CBool(CByte(-0.1))) = " & getVT(CBool(CByte(-0.1))))

Those look mostly good. However I'd like to see some more tests.
Interesting case is for example empty string and a "false", "False" and
"#FALSE#" string. Also I'd really like to see tests on object default
values. See the attached patch for an example.

Those comments apply also to other patches in the series. It may be
easier to proceed if you resent just the first patch fixed and include
tests that don't depending on other patches in the series in the patch
itself.

Cheers,
Jacek

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Shuai Meng]

2014-04-21 17:18 GMT+08:00 Jacek Caban :
> Hi Shuai,
>
> On 04/21/14 03:45, Shuai Meng wrote:
> > -
> > - return return_bool(res, val);
> > - V_VT(res) = VT_EMPTY;
> > - return VariantChangeType(res, arg, VARIANT_LOCALBOOL, VT_BOOL);
>
> You can't assume that res is not NULL. If result of the function is not
> used, it will be NULL. A simple test case is like this:
>
> call CBool(0)
>
> Also, are you sure we want VARIANT_LOCALBOOL here? Did you test it
> (probably by running tests on localized Windows with translated string
> "False")?
>
> I don't quite understand what this parameter means, in fact I just copy

it from to_string(). If not VARIANT_LOCALBOOL, waht esle should be filled
here?
> Looking at CBool tests:
>
> +Call ok(CBool(Empty) = False, "CBool(Empty) = " & CBool(Empty))
> +Call ok(getVT(CBool(Empty)) = "VT_BOOL", "getVT(CBool(Empty)) = " &
> getVT(CBool(Empty)))
> +Call ok(CBool(1) = True, "CBool(1) = " & CBool(1))
> +Call ok(getVT(CBool(1)) = "VT_BOOL", "getVT(CBool(1)) = " &
> getVT(CBool(1)))
> +Call ok(CBool(CLng(0)) = False, "CBool(CLng(0)) = " & CBool(CLng(0)))
> +Call ok(getVT(CBool(CLng(0))) = "VT_BOOL", "getVT(CBool(CLng(0))) = " &
> getVT(CBool(CLng(0))))
> +Call ok(CBool(CSng(0.5)) = True, "CBool(CSng(0.5)) = " & CBool(CSng(0.5)))
> +Call ok(getVT(CBool(CSng(0.5))) = "VT_BOOL", "getVT(CBool(CSng(0.5))) = "
> & getVT(CBool(CSng(0.5))))
> +Call ok(CBool(-0.56) = True, "CBool(-0.56) = " & CBool(-0.56))
> +Call ok(getVT(CBool(-0.56)) = "VT_BOOL", "getVT(CBool(-0.56)) = " &
> getVT(CBool(-0.56)))
> +Call ok(CBool(CCur(8.55)) = True, "CBool(CCur(8.55)) = " &
> CBool(CCur(8.55)))
> +Call ok(getVT(CBool(CCur(8.55))) = "VT_BOOL", "getVT(CBool(CCur(8.55))) =
> " & getVT(CBool(CCur(8.55))))
> +Call ok(CBool(CDate(0)) = False, "CBool(CDate(0)) = " & CBool(CDate(0)))
> +Call ok(getVT(CBool(CDate(0))) = "VT_BOOL", "getVT(CBool(CDate(0))) = " &
> getVT(CBool(CDate(0))))
> +Call ok(CBool(CDate(3.33)) = True, "CBool(CDate(3.33)) = " &
> CBool(CDate(3.33)))
> +Call ok(getVT(CBool(CDate(3.33))) = "VT_BOOL", "getVT(CBool(CDate(3.33)))
> = " & getVT(CBool(CDate(3.33))))
> +Call ok(CBool("6666.78") = True, "CBool(""6666.78"") = " &
> CBool("6666.78"))
> +Call ok(getVT(CBool("6666.78")) = "VT_BOOL", "getVT(CBool(""6666.78"")) =
> " & getVT(CBool("6666.78")))
> +Call ok(CBool(CByte(-0.1)) = False, "CBool(CByte(-0.1)) = " &
> CBool(CByte(-0.1)))
> +Call ok(getVT(CBool(CByte(-0.1))) = "VT_BOOL", "getVT(CBool(CByte(-0.1)))
> = " & getVT(CBool(CByte(-0.1))))
>
> Those look mostly good. However I'd like to see some more tests.
> Interesting case is for example empty string and a "false", "False" and
> "#FALSE#" string. Also I'd really like to see tests on object default
> values. See the attached patch for an example.
>
> Those comments apply also to other patches in the series. It may be
> easier to proceed if you resent just the first patch fixed and include
> tests that don't depending on other patches in the series in the patch
> itself.
>
> Cheers,
> Jacek

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Piotr Caban]

On 04/28/14 17:26, Shuai Meng wrote:
> 2014-04-21 17:18 GMT+08:00 Jacek Caban <[e-mail]
> >:
> Hi Shuai,
>
> On 04/21/14 03:45, Shuai Meng wrote:
>  > -
>  > -    return return_bool(res, val);
>  > +    V_VT(res) = VT_EMPTY;
>  > +    return VariantChangeType(res, arg, VARIANT_LOCALBOOL, VT_BOOL);
>
> You can't assume that res is not NULL. If result of the function is not
> used, it will be NULL. A simple test case is like this:
>
> call CBool(0)
>
> Also, are you sure we want VARIANT_LOCALBOOL here? Did you test it
> (probably by running tests on localized Windows with translated string
> "False")?
>
> I don't quite understand what this parameter means, in fact I just
> copy it from to_string(). If not VARIANT_LOCALBOOL, waht esle should be
> filled here?

I've checked what happens on non-english locale and CStr(True) = "True". 
So the flag is used incorrectly in CStr implementation. CBool is also 
failing when localized string is passed.

You can use 0 as flag in VariantChangeType.

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Shuai Meng]

2014-04-29 0:04 GMT+08:00 Piotr Caban :
> On 04/28/14 17:26, Shuai Meng wrote:
> > 2014-04-21 17:18 GMT+08:00 Jacek Caban <[e-mail]
> > >:
> > Hi Shuai,
> >
> > On 04/21/14 03:45, Shuai Meng wrote:
> >  > -
> >  > -    return return_bool(res, val);
> >  > +    V_VT(res) = VT_EMPTY;
> >  > +    return VariantChangeType(res, arg, VARIANT_LOCALBOOL,
> >
> > VT_BOOL);
> > You can't assume that res is not NULL. If result of the function is
> >
> > not
> >     used, it will be NULL. A simple test case is like this:
> > call CBool(0)
> >
> > Also, are you sure we want VARIANT_LOCALBOOL here? Did you test it
> > (probably by running tests on localized Windows with translated string
> > "False")?
> >
> > I don't quite understand what this parameter means, in fact I just
> > copy it from to_string(). If not VARIANT_LOCALBOOL, waht esle should be
> > filled here?
>
> I've checked what happens on non-english locale and CStr(True) = "True".
> So the flag is used incorrectly in CStr implementation. CBool is also
> failing when localized string is passed.
>
> You can use 0 as flag in VariantChangeType.

I've fixed this, but another problem appears. I put my new patch in the
attachment, along with the test result of wine. It fails when tested in
wine, while normal in my windows. I can't understand, this patch is almost
the same with the previous one.

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Shuai Meng]

2014-04-21 17:18 GMT+08:00 Jacek Caban :
> Hi Shuai,
>
> On 04/21/14 03:45, Shuai Meng wrote:
> > -
> > - return return_bool(res, val);
> > - V_VT(res) = VT_EMPTY;
> > - return VariantChangeType(res, arg, VARIANT_LOCALBOOL, VT_BOOL);
>
> You can't assume that res is not NULL. If result of the function is not
> used, it will be NULL. A simple test case is like this:
>
> call CBool(0)
>
> Also, are you sure we want VARIANT_LOCALBOOL here? Did you test it
> (probably by running tests on localized Windows with translated string
> "False")?
>
> Looking at CBool tests:
>
> +Call ok(CBool(Empty) = False, "CBool(Empty) = " & CBool(Empty))
> +Call ok(getVT(CBool(Empty)) = "VT_BOOL", "getVT(CBool(Empty)) = " &
> getVT(CBool(Empty)))
> +Call ok(CBool(1) = True, "CBool(1) = " & CBool(1))
> +Call ok(getVT(CBool(1)) = "VT_BOOL", "getVT(CBool(1)) = " &
> getVT(CBool(1)))
> +Call ok(CBool(CLng(0)) = False, "CBool(CLng(0)) = " & CBool(CLng(0)))
> +Call ok(getVT(CBool(CLng(0))) = "VT_BOOL", "getVT(CBool(CLng(0))) = " &
> getVT(CBool(CLng(0))))
> +Call ok(CBool(CSng(0.5)) = True, "CBool(CSng(0.5)) = " & CBool(CSng(0.5)))
> +Call ok(getVT(CBool(CSng(0.5))) = "VT_BOOL", "getVT(CBool(CSng(0.5))) = "
> & getVT(CBool(CSng(0.5))))
> +Call ok(CBool(-0.56) = True, "CBool(-0.56) = " & CBool(-0.56))
> +Call ok(getVT(CBool(-0.56)) = "VT_BOOL", "getVT(CBool(-0.56)) = " &
> getVT(CBool(-0.56)))
> +Call ok(CBool(CCur(8.55)) = True, "CBool(CCur(8.55)) = " &
> CBool(CCur(8.55)))
> +Call ok(getVT(CBool(CCur(8.55))) = "VT_BOOL", "getVT(CBool(CCur(8.55))) =
> " & getVT(CBool(CCur(8.55))))
> +Call ok(CBool(CDate(0)) = False, "CBool(CDate(0)) = " & CBool(CDate(0)))
> +Call ok(getVT(CBool(CDate(0))) = "VT_BOOL", "getVT(CBool(CDate(0))) = " &
> getVT(CBool(CDate(0))))
> +Call ok(CBool(CDate(3.33)) = True, "CBool(CDate(3.33)) = " &
> CBool(CDate(3.33)))
> +Call ok(getVT(CBool(CDate(3.33))) = "VT_BOOL", "getVT(CBool(CDate(3.33)))
> = " & getVT(CBool(CDate(3.33))))
> +Call ok(CBool("6666.78") = True, "CBool(""6666.78"") = " &
> CBool("6666.78"))
> +Call ok(getVT(CBool("6666.78")) = "VT_BOOL", "getVT(CBool(""6666.78"")) =
> " & getVT(CBool("6666.78")))
> +Call ok(CBool(CByte(-0.1)) = False, "CBool(CByte(-0.1)) = " &
> CBool(CByte(-0.1)))
> +Call ok(getVT(CBool(CByte(-0.1))) = "VT_BOOL", "getVT(CBool(CByte(-0.1)))
> = " & getVT(CBool(CByte(-0.1))))
>
> Those look mostly good. However I'd like to see some more tests.
> Interesting case is for example empty string and a "false", "False" and
> "#FALSE#" string.

These strings can't pass tests in testbot, I have tried. And I find a line
in http://msdn.microsoft.com/en-us/library/2k9sfx3c(v=vs.84).aspx  says
that "If *expression* can't be interpreted as a numeric value, a run-time
error occurs."

Also I'd really like to see tests on object default
> values. See the attached patch for an example.

I copied the example, and found that though tests on object default values
passed in testbot, they can't pass in wine. I think this is because
VariantChangeType function can't change an object into a VT_BOOL. So I
delete them.
> Those comments apply also to other patches in the series. It may be
> easier to proceed if you resent just the first patch fixed and include
> tests that don't depending on other patches in the series in the patch
> itself.
>
> Cheers,
> Jacek

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Jacek Caban]

On 04/29/14 13:11, Shuai Meng wrote:
> Also I'd really like to see tests on object default
> values. See the attached patch for an example.
>
>
> I copied the example, and found that though tests on object default
> values passed in testbot, they can't pass in wine. I think this is
> because VariantChangeType function can't change an object into a
> VT_BOOL. So I delete them.

This patch should fix the problem:
http://source.winehq.org/patches/data/104259

Please add those tests back when you resend.

Cheers,
Jacek

[PATCH 01/10] vbscript: Fixed CBool(try 3)

[Shuai Meng]

ok, I will add those tests.

2014-04-30 17:37 GMT+08:00 Jacek Caban :
> On 04/29/14 13:11, Shuai Meng wrote:
>
> Also I'd really like to see tests on object default
> > values. See the attached patch for an example.
>
> I copied the example, and found that though tests on object default
> values passed in testbot, they can't pass in wine. I think this is because
> VariantChangeType function can't change an object into a VT_BOOL. So I
> delete them.
>
> This patch should fix the problem:
> http://source.winehq.org/patches/data/104259
>
> Please add those tests back when you resend.
>
> Cheers,
> Jacek