fix warning from scalar --> int32 conversion

fix warning from scalar --> int32 conversion


BUG=

Committed: https://code.google.com/p/skia/source/detail?r=7061

[brian2, 2013-01-07 17:55:56]

On 2013/01/07 17:47:18, Humper wrote:

Lgtm

[Steve VanDeBogart, 2013-01-07 18:20:57]

On Jan 7, 2013 9:47 AM, <humper@google.com> wrote:
>
> Reviewers: bsalomon, Steve VanDeBogart,
>
> Description:
> fix warning from scalar --> int32 conversion
>
>
> BUG=
>
>
> Please review this at https://codereview.appspot.com/7065050/
>
> Affected files:
>   M src/ports/SkFontHost_mac_coretext.cpp
>
>
> Index: src/ports/SkFontHost_mac_coretext.cpp
> diff --git a/src/ports/SkFontHost_mac_coretext.cpp
b/src/ports/SkFontHost_mac_coretext.cpp
> index
527b9816ce2af7bf898f1cc04188b547cc39cf85..6a17d75e4f906c3836b6e4316e11aff987eb7301
100644
> --- a/src/ports/SkFontHost_mac_coretext.cpp
> +++ b/src/ports/SkFontHost_mac_coretext.cpp
> @@ -270,6 +270,10 @@ static SkScalar CGToScalar(CGFloat cgFloat) {
>      }
>  }
>
> +static int32_t CGRoundToInt(CGFloat cgFloat) {
> +    return (int32_t) floor(cgFloat + 0.5);

I think the style guide now says to use C++ style casts.

> +}
> +
>  static CGAffineTransform MatrixToCGAffineTransform(const SkMatrix&
matrix,
>                                                     SkScalar sx =
SK_Scalar1,
>                                                     SkScalar sy =
SK_Scalar1) {
> @@ -1525,10 +1529,10 @@ SkAdvancedTypefaceMetrics*
SkFontHost::GetAdvancedTypefaceMetrics(
>      info->fCapHeight = (int16_t) CTFontGetCapHeight(ctFont);
>      CGRect bbox = CTFontGetBoundingBox(ctFont);
>      info->fBBox = SkIRect::MakeLTRB(

Since this is calculating a bounding box, the left and top should probably
round down and the right and bottom should round up.

> -            CGToScalar(CGRectGetMinX_inline(bbox)),   // Left
> -            CGToScalar(CGRectGetMaxY_inline(bbox)),   // Top
> -            CGToScalar(CGRectGetMaxX_inline(bbox)),   // Right
> -            CGToScalar(CGRectGetMinY_inline(bbox)));  // Bottom
> +            CGRoundToInt(CGRectGetMinX_inline(bbox)),   // Left
> +            CGRoundToInt(CGRectGetMaxY_inline(bbox)),   // Top
> +            CGRoundToInt(CGRectGetMaxX_inline(bbox)),   // Right
> +            CGRoundToInt(CGRectGetMinY_inline(bbox)));  // Bottom
>
>      // Figure out a good guess for StemV - Min width of i, I, !, 1.
>      // This probably isn't very good with an italic font.
>
>

[Humper, 2013-01-07 18:23:35]

On 2013/01/07 18:20:57, Steve VanDeBogart wrote:
> On Jan 7, 2013 9:47 AM, <mailto:humper@google.com> wrote:
> >
> > Reviewers: bsalomon, Steve VanDeBogart,
> >
> > Description:
> > fix warning from scalar --> int32 conversion
> >
> >
> > BUG=
> >
> >
> > Please review this at https://codereview.appspot.com/7065050/
> >
> > Affected files:
> >   M src/ports/SkFontHost_mac_coretext.cpp
> >
> >
> > Index: src/ports/SkFontHost_mac_coretext.cpp
> > diff --git a/src/ports/SkFontHost_mac_coretext.cpp
> b/src/ports/SkFontHost_mac_coretext.cpp
> > index
>
527b9816ce2af7bf898f1cc04188b547cc39cf85..6a17d75e4f906c3836b6e4316e11aff987eb7301
> 100644
> > --- a/src/ports/SkFontHost_mac_coretext.cpp
> > +++ b/src/ports/SkFontHost_mac_coretext.cpp
> > @@ -270,6 +270,10 @@ static SkScalar CGToScalar(CGFloat cgFloat) {
> >      }
> >  }
> >
> > +static int32_t CGRoundToInt(CGFloat cgFloat) {
> > +    return (int32_t) floor(cgFloat + 0.5);
> 
> I think the style guide now says to use C++ style casts.
> 
> > +}
> > +
> >  static CGAffineTransform MatrixToCGAffineTransform(const SkMatrix&
> matrix,
> >                                                     SkScalar sx =
> SK_Scalar1,
> >                                                     SkScalar sy =
> SK_Scalar1) {
> > @@ -1525,10 +1529,10 @@ SkAdvancedTypefaceMetrics*
> SkFontHost::GetAdvancedTypefaceMetrics(
> >      info->fCapHeight = (int16_t) CTFontGetCapHeight(ctFont);
> >      CGRect bbox = CTFontGetBoundingBox(ctFont);
> >      info->fBBox = SkIRect::MakeLTRB(
> 
> Since this is calculating a bounding box, the left and top should probably
> round down and the right and bottom should round up.
> 
> > -            CGToScalar(CGRectGetMinX_inline(bbox)),   // Left
> > -            CGToScalar(CGRectGetMaxY_inline(bbox)),   // Top
> > -            CGToScalar(CGRectGetMaxX_inline(bbox)),   // Right
> > -            CGToScalar(CGRectGetMinY_inline(bbox)));  // Bottom
> > +            CGRoundToInt(CGRectGetMinX_inline(bbox)),   // Left
> > +            CGRoundToInt(CGRectGetMaxY_inline(bbox)),   // Top
> > +            CGRoundToInt(CGRectGetMaxX_inline(bbox)),   // Right
> > +            CGRoundToInt(CGRectGetMinY_inline(bbox)));  // Bottom
> >
> >      // Figure out a good guess for StemV - Min width of i, I, !, 1.
> >      // This probably isn't very good with an italic font.
> >
> >

Thanks, will update.

[Humper, 2013-01-07 18:31:24]

Made Steve's changes for the font bbox warning, and also removed every single
warning from the release build...

[bsalomon, 2013-01-07 18:38:25]

The other files all lgtm.

We should double check with Mike about the rounding... it was trunc, then he
suggested round, now its round-out. I'm not sure which is correct. On a minor
note I'd name the macros CGFloorToInt, CGCeilToInt just to emphasize the return
type.

https://codereview.appspot.com/7065050/diff/6031/src/ports/SkFontHost_mac_cor...
File src/ports/SkFontHost_mac_coretext.cpp (right):

https://codereview.appspot.com/7065050/diff/6031/src/ports/SkFontHost_mac_cor...
src/ports/SkFontHost_mac_coretext.cpp:274: return int32_t(cgFloat);
Isn't this trunc? or are the numbers always >= 0?

[Humper, 2013-01-07 18:40:18]

https://codereview.appspot.com/7065050/diff/6031/src/ports/SkFontHost_mac_cor...
File src/ports/SkFontHost_mac_coretext.cpp (right):

https://codereview.appspot.com/7065050/diff/6031/src/ports/SkFontHost_mac_cor...
src/ports/SkFontHost_mac_coretext.cpp:274: return int32_t(cgFloat);
On 2013/01/07 18:38:25, bsalomon wrote:
> Isn't this trunc? or are the numbers always >= 0?

Yeah, this is just wrong.  I'll fix it to be floor and ask Mike how he feels
about the trunc --> round --> expand rollercoaster.

[reed1, 2013-01-07 18:45:30]

I will defer to Vandebo for what the "correct" behavior is, given that this
return value is for PDF.

Given that we may need some choices, I think the clearest way to code this
(reversing my earlier suggestion) is.

SkRect r;
r.set(CGToScalar(...));
// Now we can use the existing round/out/etc. stuff
r.roundOut(&bbox) or r.round(&bbox), or ...