Re: [PATCH] vbscript: Implemented RGB

[Nikolay Sivov]

On 09/18/2014 08:46 PM, Shuai Meng wrote:
> +    LONG total, color[3];//color[0] for red, color[1] for green, color[2] for blue.

This could be just 'int', as you use to_int().
> static HRESULT return_int(VARIANT *res, int val)
>   {
> -    if((short)val == val)
> -        return return_short(res, val);
> -

That's a questionable thing to do as it affects all other places where 
this helper is used.
> +        if(color[i] > 255)
> +            color[i] = 255;

This needs a test too.
> +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];

RGB() macro does the same thing.

[PATCH] vbscript: Implemented RGB

[Shuai Meng]

Thanks for commenting~

2014-09-19 2:09 GMT+08:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
> On 09/18/2014 08:46 PM, Shuai Meng wrote:
> > +    LONG total, color[3];//color[0] for red, color[1] for green,
> > color[2] for blue.
>
> This could be just 'int', as you use to_int().
>
> static HRESULT return_int(VARIANT *res, int val)
> > {
> > -    if((short)val == val)
> > -        return return_short(res, val);
> > -
>
> That's a questionable thing to do as it affects all other places where
> this helper is used.

You are right, but I will check that whether all the functions who call
to_int() use these codes, I think these are unnecessary, cause we should
use to_int and to_short seperately in order to get return values of
different types, i.e. VT_I2 and VT_I4, especially for the smaller numbers
such as 0.
> +        if(color[i] > 255)
> > +            color[i] = 255;
>
> This needs a test too.
>
> Yeah, good idea.

> +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];
> >
>
> RGB() macro does the same thing.
>
> I don't quite understand this line..will you explain it in detail please?

[PATCH] vbscript: Implemented RGB

[Shuai Meng]

2014-09-19 19:52 GMT+08:00 Shuai Meng <mengshuaicalendr(a)gmail.com>:
> Thanks for commenting~
>
> 2014-09-19 2:09 GMT+08:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
> > On 09/18/2014 08:46 PM, Shuai Meng wrote:
> > > +    LONG total, color[3];//color[0] for red, color[1] for green,
> > > color[2] for blue.
> >
> > This could be just 'int', as you use to_int().
> >
> > static HRESULT return_int(VARIANT *res, int val)
> > > {
> > > -    if((short)val == val)
> > > -        return return_short(res, val);
> > > -
> >
> > That's a questionable thing to do as it affects all other places where
> > this helper is used.
>
> You are right, but I will check that whether all the functions who call
> to_int() use these codes, I think these are unnecessary, cause we should
> use to_int and to_short seperately in order to get return values of
> different types, i.e. VT_I2 and VT_I4, especially for the smaller numbers
> such as 0.
> > +        if(color[i] > 255)
> > > +            color[i] = 255;
> >
> > This needs a test too.
> >
> > Yeah, good idea.
> > +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];
> > >
> >
> > RGB() macro does the same thing.
> >
> > I don't quite understand this line..will you explain it in detail please?

OK, forgive my poor English, I know what "macro" means.

[PATCH] vbscript: Implemented RGB

[Shuai Meng]

2014-09-19 19:52 GMT+08:00 Shuai Meng <mengshuaicalendr(a)gmail.com>:
> Thanks for commenting~
>
> 2014-09-19 2:09 GMT+08:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
> > On 09/18/2014 08:46 PM, Shuai Meng wrote:
> > > +    LONG total, color[3];//color[0] for red, color[1] for green,
> > > color[2] for blue.
> >
> > This could be just 'int', as you use to_int().
> >
> > static HRESULT return_int(VARIANT *res, int val)
> > > {
> > > -    if((short)val == val)
> > > -        return return_short(res, val);
> > > -
> >
> > That's a questionable thing to do as it affects all other places where
> > this helper is used.
>
> You are right, but I will check that whether all the functions who call
> to_int() use these codes, I think these are unnecessary, cause we should
> use to_int and to_short seperately in order to get return values of
> different types, i.e. VT_I2 and VT_I4, especially for the smaller numbers
> such as 0.

Uh, I mean return_int and return_short.
> > +        if(color[i] > 255)
> > > +            color[i] = 255;
> >
> > This needs a test too.
> >
> > Yeah, good idea.
> > +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];
> > >
> >
> > RGB() macro does the same thing.
> >
> > I don't quite understand this line..will you explain it in detail please?

[PATCH] vbscript: Implemented RGB

[GOUJON Alexandre]

On 09/19/2014 02:14 PM, Shuai Meng wrote:
> +   total = color[0] + 256 * color[1] + 256 * 256 * color[2];
>
> RGB() macro does the same thing.
>
> I don't quite understand this line..will you explain it in detail please?

See http://source.winehq.org/source/include/wingdi.h#0434
So total = color[0] + 256 * color[1] + 256*256*color[2] is the same as 
total = RGB(color[0], color[1], color[2])

[PATCH] vbscript: Implemented RGB

[Shuai Meng]

2014-09-19 21:02 GMT+08:00 GOUJON Alexandre <ale.goujon(a)gmail.com>:
> On 09/19/2014 02:14 PM, Shuai Meng wrote:
>
> +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];
> > >
> >
> > RGB() macro does the same thing.
> >
> > I don't quite understand this line..will you explain it in detail
> > please?
>
> See http://source.winehq.org/source/include/wingdi.h#0434
> So total = color[0] + 256 * color[1] + 256*256*color[2] is the same as
> total = RGB(color[0], color[1], color[2])

Thank you, I have found  that.

[PATCH] vbscript: Implemented RGB

[Nikolay Sivov]

On Fri, Sep 19, 2014 at 4:02 PM, GOUJON Alexandre <ale.goujon(a)gmail.com>
wrote:
> On 09/19/2014 02:14 PM, Shuai Meng wrote:
>
> +    total = color[0] + 256 * color[1] + 256 * 256 * color[2];
> > >
> >
> > RGB() macro does the same thing.
> >
> > I don't quite understand this line..will you explain it in detail
> > please?
>
> See http://source.winehq.org/source/include/wingdi.h#0434
> So total = color[0] + 256 * color[1] + 256*256*color[2] is the same as
> total = RGB(color[0], color[1], color[2])
>
> Right, it's also better to rename color component variables imho.