Re: [PATCH] vbscript: Implemented IsNumeric

[Dmitry Timoshkov]

Shuai Meng <mengshuaicalendr(a)gmail.com> wrote:
> +        switch(V_VT(arg)) {
> +        case VT_UI1:
> +        case VT_I2:
> +        case VT_I4:
> +        case VT_I8:
> +        case VT_R4:
> +        case VT_R8:
> +        case VT_BOOL:
> +        case VT_EMPTY:
> +        case VT_CY:
> +            V_BOOL(res) = VARIANT_TRUE;
> +            break;

This list is far from being complete. It seems that it should also
contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
taken into account.

-- 
Dmitry.

[PATCH] vbscript: Implemented IsNumeric

[Shuai Meng]

Thanks for commenting, I'm working on it now.


2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru>:
> Shuai Meng <mengshuaicalendr(a)gmail.com> wrote:
> > +        switch(V_VT(arg)) {
> > +        case VT_UI1:
> > +        case VT_I2:
> > +        case VT_I4:
> > +        case VT_I8:
> > +        case VT_R4:
> > +        case VT_R8:
> > +        case VT_BOOL:
> > +        case VT_EMPTY:
> > +        case VT_CY:
> > +            V_BOOL(res) = VARIANT_TRUE;
> > +            break;
>
> This list is far from being complete. It seems that it should also
> contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
> VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
> taken into account.
>
> --
> Dmitry.

[PATCH] vbscript: Implemented IsNumeric

[Shuai Meng]

2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru>:
> Shuai Meng <mengshuaicalendr(a)gmail.com> wrote:
> > +        switch(V_VT(arg)) {
> > +        case VT_UI1:
> > +        case VT_I2:
> > +        case VT_I4:
> > +        case VT_I8:
> > +        case VT_R4:
> > +        case VT_R8:
> > +        case VT_BOOL:
> > +        case VT_EMPTY:
> > +        case VT_CY:
> > +            V_BOOL(res) = VARIANT_TRUE;
> > +            break;
>
> This list is far from being complete. It seems that it should also
> contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
> VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
> taken into account.
>
> --
> Dmitry.

Hi, I think over it again. Let's face the fact: what are all the possible
results of  switch(V_VT(arg)) ? Will VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT,
VT_UINT and so on exist? My answer is no. arg comes from users' input,
which means it only belongs to the  subtypes of variant, see here:
http://msdn.microsoft.com/en-us/library/9e7a57cf(v=vs.84).aspx

[PATCH] vbscript: Implemented IsNumeric

[Jacek Caban]

On 07/18/14 04:11, Shuai Meng wrote:
> 2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru
> <mailto:dmitry(a)baikal.ru>>:
>
> Shuai Meng <mengshuaicalendr(a)gmail.com
>     <mailto:mengshuaicalendr(a)gmail.com>> wrote:
>
> > +        switch(V_VT(arg)) {
>     > +        case VT_UI1:
>     > +        case VT_I2:
>     > +        case VT_I4:
>     > +        case VT_I8:
>     > +        case VT_R4:
>     > +        case VT_R8:
>     > +        case VT_BOOL:
>     > +        case VT_EMPTY:
>     > +        case VT_CY:
>     > +            V_BOOL(res) = VARIANT_TRUE;
>     > +            break;
>
> This list is far from being complete. It seems that it should also
>     contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
>     VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
>     taken into account.
>
> --
>     Dmitry.
>
> Hi, I think over it again. Let's face the fact: what are all the
> possible results of  switch(V_VT(arg)) ? Will VT_UI2, VT_UI4, VT_UI8,
> VT_I1, VT_INT, VT_UINT and so on exist? My answer is no. arg comes
> from users' input, which means it only belongs to the  subtypes of
> variant, see here:
> http://msdn.microsoft.com/en-us/library/9e7a57cf(v=vs.84).aspx
> <http://msdn.microsoft.com/en-us/library/9e7a57cf%28v=vs.84%29.aspx>

No, your answer is wrong. VBScript code may be called from non-VBScript
(and the other way around) and then any type of VARIANT may be passed
around. Also, please add a test with an object having default value,
which is of numeric type. I have a feeling that the right thing to do
here is to call to_double and see if it fails or not, but this needs
tests to confirm.

Jacek

[PATCH] vbscript: Implemented IsNumeric

[Shuai Meng]

Thanks all for commenting, I changed the implementation and put the codes
here:
https://github.com/Shuai-Meng/wine/commit/bb212763ffd639a238b5a47f2c6a27047a...
It passed on both winxp and wine. But this patch still needs your comments.



2014-07-18 17:30 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
> On 07/18/14 04:11, Shuai Meng wrote:
>
> 2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru>:
> > Shuai Meng <mengshuaicalendr(a)gmail.com> wrote:
> > > +        switch(V_VT(arg)) {
> > > +        case VT_UI1:
> > > +        case VT_I2:
> > > +        case VT_I4:
> > > +        case VT_I8:
> > > +        case VT_R4:
> > > +        case VT_R8:
> > > +        case VT_BOOL:
> > > +        case VT_EMPTY:
> > > +        case VT_CY:
> > > +            V_BOOL(res) = VARIANT_TRUE;
> > > +            break;
> >
> > This list is far from being complete. It seems that it should also
> > contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
> > VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
> > taken into account.
> >
> > --
> > Dmitry.
>
> Hi, I think over it again. Let's face the fact: what are all the possible
> results of  switch(V_VT(arg)) ? Will VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT,
> VT_UINT and so on exist? My answer is no. arg comes from users' input,
> which means it only belongs to the  subtypes of variant, see here:
> http://msdn.microsoft.com/en-us/library/9e7a57cf(v=vs.84).aspx
>
> No, your answer is wrong. VBScript code may be called from non-VBScript
> (and the other way around) and then any type of VARIANT may be passed
> around. Also, please add a test with an object having default value, which
> is of numeric type. I have a feeling that the right thing to do here is to
> call to_double and see if it fails or not, but this needs tests to confirm.
>
> Jacek

[PATCH] vbscript: Implemented IsNumeric

[Jacek Caban]

Hi Shuai,

A few comments:

- We probably should propagate unrelated errors. So if FAILED(hres) &&
hres != DISP_E_TYPEMISMATCH, we should return hres

    if(FAILED(hres))
        return return_bool(res, FALSE);
    else
        return return_bool(res, TRUE);

Why not:
    return return_bool(res, SUCCEEDED(hres))

static HRESULT return_bool(VARIANT *res, BOOL val)
{
    if(res) {
        V_VT(res) = VT_BOOL;
        V_BOOL(res) = val;
    }
    return S_OK;
}

VARIANT_BOOL uses VARIANT_TRUE/VARIANT_FALSE values, so this should be:
V_BOOL(res) = val ? VARIANT_TRUE : VARIANT_FALSE;

Cheers,
Jacek



On 07/18/14 17:29, Shuai Meng wrote:
> Thanks all for commenting, I changed the implementation and put the
> codes
> here: https://github.com/Shuai-Meng/wine/commit/bb212763ffd639a238b5a47f2c6a27047a...
>
> It passed on both winxp and wine. But this patch still needs your
> comments.
>
> 2014-07-18 17:30 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com
> <mailto:jacek(a)codeweavers.com>>:
>
> On 07/18/14 04:11, Shuai Meng wrote:
> > 2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru
> >     <mailto:dmitry(a)baikal.ru>>:
> >
> > Shuai Meng <mengshuaicalendr(a)gmail.com
> >         <mailto:mengshuaicalendr(a)gmail.com>> wrote:
> >
> > > +        switch(V_VT(arg)) {
> >         > +        case VT_UI1:
> >         > +        case VT_I2:
> >         > +        case VT_I4:
> >         > +        case VT_I8:
> >         > +        case VT_R4:
> >         > +        case VT_R8:
> >         > +        case VT_BOOL:
> >         > +        case VT_EMPTY:
> >         > +        case VT_CY:
> >         > +            V_BOOL(res) = VARIANT_TRUE;
> >         > +            break;
> >
> > This list is far from being complete. It seems that it should
> >         also
> >         contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
> >         VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should be
> >         taken into account.
> >
> > --
> >         Dmitry.
> >
> > Hi, I think over it again. Let's face the fact: what are all the
> >     possible results of  switch(V_VT(arg)) ? Will VT_UI2, VT_UI4,
> >     VT_UI8, VT_I1, VT_INT, VT_UINT and so on exist? My answer is no.
> >     arg comes from users' input, which means it only belongs to the
> >      subtypes of variant, see here:
> >     http://msdn.microsoft.com/en-us/library/9e7a57cf(v=vs.84).aspx
> >     <http://msdn.microsoft.com/en-us/library/9e7a57cf%28v=vs.84%29.aspx>
>
> No, your answer is wrong. VBScript code may be called from
>     non-VBScript (and the other way around) and then any type of
>     VARIANT may be passed around. Also, please add a test with an
>     object having default value, which is of numeric type. I have a
>     feeling that the right thing to do here is to call to_double and
>     see if it fails or not, but this needs tests to confirm.
>
> Jacek

[PATCH] vbscript: Implemented IsNumeric

[Shuai Meng]

Thanks very much for commenting~


2014-07-19 0:22 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
> Hi Shuai,
>
> A few comments:
>
> - We probably should propagate unrelated errors. So if FAILED(hres) &&
> hres != DISP_E_TYPEMISMATCH, we should return hres
>
> if(FAILED(hres))
>         return return_bool(res, FALSE);
>     else
>         return return_bool(res, TRUE);
>
> Why not:
>     return return_bool(res, SUCCEEDED(hres))
>
> static HRESULT return_bool(VARIANT *res, BOOL val)
> {
>     if(res) {
>         V_VT(res) = VT_BOOL;
>         V_BOOL(res) = val;
>     }
>     return S_OK;
> }
>
> VARIANT_BOOL uses VARIANT_TRUE/VARIANT_FALSE values, so this should be:
> V_BOOL(res) = val ? VARIANT_TRUE : VARIANT_FALSE;
>
> Cheers,
> Jacek
>
> On 07/18/14 17:29, Shuai Meng wrote:
> > Thanks all for commenting, I changed the implementation and put the
> > codes
> > here:
> > https://github.com/Shuai-Meng/wine/commit/bb212763ffd639a238b5a47f2c6a27047a...
> >
> > It passed on both winxp and wine. But this patch still needs your
> > comments.
> >
> > 2014-07-18 17:30 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com
> > <mailto:jacek(a)codeweavers.com>>:
> >
> > On 07/18/14 04:11, Shuai Meng wrote:
> > > 2014-07-17 11:11 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru
> > >     <mailto:dmitry(a)baikal.ru>>:
> > >
> > > Shuai Meng <mengshuaicalendr(a)gmail.com
> > >         <mailto:mengshuaicalendr(a)gmail.com>> wrote:
> > >
> > > > +        switch(V_VT(arg)) {
> > >         > +        case VT_UI1:
> > >         > +        case VT_I2:
> > >         > +        case VT_I4:
> > >         > +        case VT_I8:
> > >         > +        case VT_R4:
> > >         > +        case VT_R8:
> > >         > +        case VT_BOOL:
> > >         > +        case VT_EMPTY:
> > >         > +        case VT_CY:
> > >         > +            V_BOOL(res) = VARIANT_TRUE;
> > >         > +            break;
> > >
> > > This list is far from being complete. It seems that it should
> > >         also
> > >         contain at least VT_UI2, VT_UI4, VT_UI8, VT_I1, VT_INT, VT_UINT.
> > >         VT_VECTOR, VT_ARRAY and VT_BYREF modifers probably also should
> >
> > be
> > > taken into account.
> > >
> > > --
> > >         Dmitry.
> > >
> > > Hi, I think over it again. Let's face the fact: what are all the
> > >     possible results of  switch(V_VT(arg)) ? Will VT_UI2, VT_UI4,
> > >     VT_UI8, VT_I1, VT_INT, VT_UINT and so on exist? My answer is no.
> > >     arg comes from users' input, which means it only belongs to the
> > >      subtypes of variant, see here:
> > >     http://msdn.microsoft.com/en-us/library/9e7a57cf(v=vs.84).aspx
> > >     <http://msdn.microsoft.com/en-us/library/9e7a57cf%28v=vs.84%29.aspx
> >
> > No, your answer is wrong. VBScript code may be called from
> >     non-VBScript (and the other way around) and then any type of
> >     VARIANT may be passed around. Also, please add a test with an
> >     object having default value, which is of numeric type. I have a
> >     feeling that the right thing to do here is to call to_double and
> >     see if it fails or not, but this needs tests to confirm.
> >
> > Jacek