Re: usp10: Use BOOL type where appropriate

[Nikolay Sivov]

On 2/26/2014 11:12, Frédéric Delanoy wrote:
> cf. http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
>
> dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
>   dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
>   dlls/usp10/usp10.c    | 26 +++++++++++++-------------
>   3 files changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> index 166d823..5b3bdc7 100644
> --- a/dlls/usp10/opentype.c
> +++ b/dlls/usp10/opentype.c
> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache *psc, const WORD *pwGlyphs, cons
>           {
>               case 0:
>               case BaseGlyph:
> -
>            pGlyphProp[i].sva.fClusterStart = 1;
> -
>            pGlyphProp[i].sva.fDiacritic = 0;
> -
>            pGlyphProp[i].sva.fZeroWidth = 0;
> -
>            pGlyphProp[i].sva.fClusterStart = TRUE;
> -
>            pGlyphProp[i].sva.fDiacritic = FALSE;
> -
>            pGlyphProp[i].sva.fZeroWidth = FALSE;
>             break;

I think it's a wrong change.Even if msdn says you can use TRUE/FALSE 
here, field is not defined as of 'BOOL' type.

usp10: Use BOOL type where appropriate

[Frédéric Delanoy]

On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov  wrote:
> On 2/26/2014 11:12, Frédéric Delanoy wrote:
> > cf.
> > http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
> >
> > dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
> >   dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
> >   dlls/usp10/usp10.c    | 26 +++++++++++++-------------
> >   3 files changed, 47 insertions(+), 47 deletions(-)
> >
> > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> > index 166d823..5b3bdc7 100644
> > --- a/dlls/usp10/opentype.c
> > +++ b/dlls/usp10/opentype.c
> > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache
> > *psc, const WORD *pwGlyphs, cons
> >           {
> >               case 0:
> >               case BaseGlyph:
> > -
> >            pGlyphProp[i].sva.fClusterStart = 1;
> > -
> >            pGlyphProp[i].sva.fDiacritic = 0;
> > -
> >            pGlyphProp[i].sva.fZeroWidth = 0;
> > -
> >            pGlyphProp[i].sva.fClusterStart = TRUE;
> > -
> >            pGlyphProp[i].sva.fDiacritic = FALSE;
> > -
> >            pGlyphProp[i].sva.fZeroWidth = FALSE;
> >             break;
>
> I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here,

MSDN says explicitly the possible values *are* TRUE and FALSE
(http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%2...)...
> field is not defined as of 'BOOL' type.

Because it defines the field as bitfield WORD:1 to get an unsigned
type; Microsoft isn't always consistent about using appropriate field
type, especially when using bitfields

usp10: Use BOOL type where appropriate

[Nikolay Sivov]

On 2/26/2014 14:11, Frédéric Delanoy wrote:
> On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov  wrote:
> > On 2/26/2014 11:12, Frédéric Delanoy wrote:
> > > cf.
> > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
> > >
> > > dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
> > >    dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
> > >    dlls/usp10/usp10.c    | 26 +++++++++++++-------------
> > >    3 files changed, 47 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> > > index 166d823..5b3bdc7 100644
> > > --- a/dlls/usp10/opentype.c
> > > +++ b/dlls/usp10/opentype.c
> > > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache
> > > *psc, const WORD *pwGlyphs, cons
> > >            {
> > >                case 0:
> > >                case BaseGlyph:
> > > -
> > >            pGlyphProp[i].sva.fClusterStart = 1;
> > > -
> > >            pGlyphProp[i].sva.fDiacritic = 0;
> > > -
> > >            pGlyphProp[i].sva.fZeroWidth = 0;
> > > -
> > >            pGlyphProp[i].sva.fClusterStart = TRUE;
> > > -
> > >            pGlyphProp[i].sva.fDiacritic = FALSE;
> > > -
> > >            pGlyphProp[i].sva.fZeroWidth = FALSE;
> > >              break;
> >
> > I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here,
>
> MSDN says explicitly the possible values *are* TRUE and FALSE
> (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%2...)...

It says a lot of things.
> > field is not defined as of 'BOOL' type.
>
> Because it defines the field as bitfield WORD:1 to get an unsigned
> type; Microsoft isn't always consistent about using appropriate field
> type, especially when using bitfields

It doesn't mean we shouldn't be consistent too. My opinion on this is 
that if it's not BOOL type explicitly - don't use BOOL-kind of constants.

usp10: Use BOOL type where appropriate

[Frédéric Delanoy]

On Wed, Feb 26, 2014 at 11:18 AM, Nikolay Sivov  wrote:
> On 2/26/2014 14:11, Frédéric Delanoy wrote:
> > On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov 
> > wrote:
> > > On 2/26/2014 11:12, Frédéric Delanoy wrote:
> > > > cf.
> > > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
> > > >
> > > > dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
> > > >    dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
> > > >    dlls/usp10/usp10.c    | 26 +++++++++++++-------------
> > > >    3 files changed, 47 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> > > > index 166d823..5b3bdc7 100644
> > > > --- a/dlls/usp10/opentype.c
> > > > +++ b/dlls/usp10/opentype.c
> > > > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache
> > > > *psc, const WORD *pwGlyphs, cons
> > > >            {
> > > >                case 0:
> > > >                case BaseGlyph:
> > > > -
> > > >            pGlyphProp[i].sva.fClusterStart = 1;
> > > > -
> > > >            pGlyphProp[i].sva.fDiacritic = 0;
> > > > -
> > > >            pGlyphProp[i].sva.fZeroWidth = 0;
> > > > -
> > > >            pGlyphProp[i].sva.fClusterStart = TRUE;
> > > > -
> > > >            pGlyphProp[i].sva.fDiacritic = FALSE;
> > > > -
> > > >            pGlyphProp[i].sva.fZeroWidth = FALSE;
> > > >              break;
> > >
> > > I think it's a wrong change.Even if msdn says you can use TRUE/FALSE
> > > here,
> >
> > MSDN says explicitly the possible values *are* TRUE and FALSE
> >
> > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%2...)...
>
> It says a lot of things.

Can you please elaborate?

E.g.

fDiacritic
    Value indicating if a glyph combines with base characters.
Possible values are defined in the following table.

Value     Meaning
    TRUE    The glyph does combine with base characters.
    FALSE  The glyph does not combine with base characters.

I don't see how much clearer that could be...

Frédéric

usp10: Use BOOL type where appropriate

[Nikolay Sivov]

On 2/26/2014 14:47, Frédéric Delanoy wrote:
> On Wed, Feb 26, 2014 at 11:18 AM, Nikolay Sivov  wrote:
> > On 2/26/2014 14:11, Frédéric Delanoy wrote:
> > > On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov 
> > > wrote:
> > > > On 2/26/2014 11:12, Frédéric Delanoy wrote:
> > > > > cf.
> > > > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
> > > > > dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
> > > > > dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
> > > > > dlls/usp10/usp10.c    | 26 +++++++++++++-------------
> > > > > 3 files changed, 47 insertions(+), 47 deletions(-)
> > > > >
> > > > > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> > > > > index 166d823..5b3bdc7 100644
> > > > > --- a/dlls/usp10/opentype.c
> > > > > +++ b/dlls/usp10/opentype.c
> > > > > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache
> > > > > *psc, const WORD *pwGlyphs, cons
> > > > >             {
> > > > >                 case 0:
> > > > >                 case BaseGlyph:
> > > > > -
> > > > >            pGlyphProp[i].sva.fClusterStart = 1;
> > > > > -
> > > > >            pGlyphProp[i].sva.fDiacritic = 0;
> > > > > -
> > > > >            pGlyphProp[i].sva.fZeroWidth = 0;
> > > > > -
> > > > >            pGlyphProp[i].sva.fClusterStart = TRUE;
> > > > > -
> > > > >            pGlyphProp[i].sva.fDiacritic = FALSE;
> > > > > -
> > > > >            pGlyphProp[i].sva.fZeroWidth = FALSE;
> > > > >               break;
> > > >
> > > > I think it's a wrong change.Even if msdn says you can use TRUE/FALSE
> > > > here,
> > >
> > > MSDN says explicitly the possible values *are* TRUE and FALSE
> > >
> > > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%2...)...
> >
> > It says a lot of things.
>
> Can you please elaborate?
>
> E.g.
>
> fDiacritic
>      Value indicating if a glyph combines with base characters.
> Possible values are defined in the following table.
>  Value     Meaning
>  TRUE    The glyph does combine with base characters.
>  FALSE  The glyph does not combine with base characters.
>
> I don't see how much clearer that could be...

My point is that you shouldn't use it because structure field type is 
not BOOL.

usp10: Use BOOL type where appropriate

[Michael Stefaniuc]

On 02/26/2014 11:11 AM, Frédéric Delanoy wrote:
> On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov  wrote:
> > On 2/26/2014 11:12, Frédéric Delanoy wrote:
> > > cf.
> > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%2...
> > >
> > > dlls/usp10/opentype.c | 32 ++++++++++++++++----------------
> > >   dlls/usp10/shape.c    | 36 ++++++++++++++++++------------------
> > >   dlls/usp10/usp10.c    | 26 +++++++++++++-------------
> > >   3 files changed, 47 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c
> > > index 166d823..5b3bdc7 100644
> > > --- a/dlls/usp10/opentype.c
> > > +++ b/dlls/usp10/opentype.c
> > > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache
> > > *psc, const WORD *pwGlyphs, cons
> > >           {
> > >               case 0:
> > >               case BaseGlyph:
> > > -
> > >            pGlyphProp[i].sva.fClusterStart = 1;
> > > -
> > >            pGlyphProp[i].sva.fDiacritic = 0;
> > > -
> > >            pGlyphProp[i].sva.fZeroWidth = 0;
> > > -
> > >            pGlyphProp[i].sva.fClusterStart = TRUE;
> > > -
> > >            pGlyphProp[i].sva.fDiacritic = FALSE;
> > > -
> > >            pGlyphProp[i].sva.fZeroWidth = FALSE;
> > >             break;
> >
> > I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here,
>
> MSDN says explicitly the possible values *are* TRUE and FALSE
> (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%2...)...
> > field is not defined as of 'BOOL' type.
>
> Because it defines the field as bitfield WORD:1 to get an unsigned
> type; Microsoft isn't always consistent about using appropriate field
> type, especially when using bitfields

BOOL is a signed integer makes no sense as a type for an 1-bit bitfield.
Yes, MS is still using it in some places.

bye
        michael