code review 940044: Freetype-Go: truetype parser initial check-in.

Freetype-Go: truetype parser initial check-in.

Parsing a malformed TTF may result in an index out-of-bounds panic,
rather than guaranteeing to return an error. This may be tightened
in the future.

[nigeltao, 2010-04-23 15:26:52]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-04-23 17:23:51]

http://codereview.appspot.com/940044/diff/2001/3001
File example/truetype/main.go (right):

http://codereview.appspot.com/940044/diff/2001/3001#newcode17
example/truetype/main.go:17: var fontfile *string = flag.String("fontfile",
"/usr/share/fonts/truetype/msttcorefonts/arial.ttf", "filename of the ttf font")
can we not use arial as the example?  whatever font you use here will be the
likeliest one for people to use and therefore to deploy. i'd rather not
encourage an overused microsoft knockoff of a weak and overused font.

lucida? lucida sans? i don't know what the other options are for linux users.

http://codereview.appspot.com/940044/diff/2001/3001#newcode30
example/truetype/main.go:30: fmt.Print("  on \n")
why space before newline?

http://codereview.appspot.com/940044/diff/2001/3003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/2001/3003#newcode20
freetype/truetype/truetype.go:20: // A GlyphIndex is a Font's encoding of a
Unicode character.
encoding doesn't seem like the right word here.

http://codereview.appspot.com/940044/diff/2001/3003#newcode23
freetype/truetype/truetype.go:23: // A Bounds holds the range of one or more
glyphs. The endpoints are inclusive.
range of what? coordinates?

http://codereview.appspot.com/940044/diff/2001/3003#newcode136
freetype/truetype/truetype.go:136: for i, x := 0, 14; i < segCount; i, x = i+1,
x+2 {
i think this would be clearer if x was defined outside the loops so we see it
just walks along the data.  it took me a while to figure that out.

another option that might work well is to define readU16 and readU32 as methods
on a *[]byte type. you could then grab a copy of the slice into a value of that
type and just read it out. no xs or offsets or magic numbers then.

http://codereview.appspot.com/940044/diff/2001/3003#newcode136
freetype/truetype/truetype.go:136: for i, x := 0, 14; i < segCount; i, x = i+1,
x+2 {
later: as i read further through the code i became confident that approach would
be good

http://codereview.appspot.com/940044/diff/2001/3003#newcode360
freetype/truetype/truetype.go:360: // A Point is a co-ordinate pair plus whether
it is `on' a contour or an
don't use this style of quotes. they look crappy in many fonts. just "" is fine.

[rsc, 2010-04-23 17:29:20]

> http://codereview.appspot.com/940044/diff/2001/3001#newcode17
> example/truetype/main.go:17: var fontfile *string =
> flag.String("fontfile",
> "/usr/share/fonts/truetype/msttcorefonts/arial.ttf", "filename of the
> ttf font")
> can we not use arial as the example?  whatever font you use here will be
> the likeliest one for people to use and therefore to deploy. i'd rather
> not encourage an overused microsoft knockoff of a weak and overused
> font.
>
> lucida? lucida sans? i don't know what the other options are for linux
> users.

Also what about non-Linux users?
It would be fine to include a copy of one or all of the Luxi fonts in
http://xorg.freedesktop.org/releases/individual/font/font-bh-ttf-1.0.0.tar.bz2
They have a non-restrictive license and are good quality
Bigelow & Holmes fonts.

I use them in Plan 9 from User Space because I can't
distribute the Lucida fonts.

Russ

[nigeltao, 2010-04-29 01:58:14]

http://codereview.appspot.com/940044/diff/2001/3001
File example/truetype/main.go (right):

http://codereview.appspot.com/940044/diff/2001/3001#newcode17
example/truetype/main.go:17: var fontfile *string = flag.String("fontfile",
"/usr/share/fonts/truetype/msttcorefonts/arial.ttf", "filename of the ttf font")
On 2010/04/23 17:23:51, r wrote:
> can we not use arial as the example?  whatever font you use here will be the
> likeliest one for people to use and therefore to deploy. i'd rather not
> encourage an overused microsoft knockoff of a weak and overused font.
> 
> lucida? lucida sans? i don't know what the other options are for linux users.
> 

Changed from Arial to the included Luxi fonts.

http://codereview.appspot.com/940044/diff/2001/3001#newcode30
example/truetype/main.go:30: fmt.Print("  on \n")
On 2010/04/23 17:23:51, r wrote:
> why space before newline?

Typo. Fixed.

http://codereview.appspot.com/940044/diff/2001/3003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/2001/3003#newcode20
freetype/truetype/truetype.go:20: // A GlyphIndex is a Font's encoding of a
Unicode character.
On 2010/04/23 17:23:51, r wrote:
> encoding doesn't seem like the right word here.

I've changed "encoding" to "index", and the type from GlyphIndex to just Index.

http://codereview.appspot.com/940044/diff/2001/3003#newcode23
freetype/truetype/truetype.go:23: // A Bounds holds the range of one or more
glyphs. The endpoints are inclusive.
On 2010/04/23 17:23:51, r wrote:
> range of what? coordinates?

Done.

http://codereview.appspot.com/940044/diff/2001/3003#newcode136
freetype/truetype/truetype.go:136: for i, x := 0, 14; i < segCount; i, x = i+1,
x+2 {
On 2010/04/23 17:23:51, r wrote:
> later: as i read further through the code i became confident that approach
would
> be good

This segment has been rewritten to use read16Slice. I think it is much clearer.

http://codereview.appspot.com/940044/diff/2001/3003#newcode360
freetype/truetype/truetype.go:360: // A Point is a co-ordinate pair plus whether
it is `on' a contour or an
On 2010/04/23 17:23:51, r wrote:
> don't use this style of quotes. they look crappy in many fonts. just "" is
fine.

Done.

[rsc, 2010-04-29 02:43:45]

> http://codereview.appspot.com/940044/diff/2001/3003#newcode360
> freetype/truetype/truetype.go:360: // A Point is a co-ordinate pair plus
> whether it is `on' a contour or an
> On 2010/04/23 17:23:51, r wrote:
>>
>> don't use this style of quotes. they look crappy in many fonts. just
>
> "" is fine.

Sorry to contradict, but please do use `` '' in doc comments.
They get turned into nice quotation marks in the HTML output
and could get turned into " " if necessary in the text output.

http://golang.org/pkg/math/#NaN is an example.

Russ

[nigeltao_gnome, 2010-04-30 00:07:39]

On 29 April 2010 12:43, Russ Cox <rsc@golang.org> wrote:
> Sorry to contradict, but please do use `` '' in doc comments.

Done.

[r, 2010-04-30 00:15:17]

http://codereview.appspot.com/940044/diff/14001/15001
File example/truetype/main.go (right):

http://codereview.appspot.com/940044/diff/14001/15001#newcode17
example/truetype/main.go:17: var fontfile *string = flag.String("fontfile",
"../../luxi-fonts/luxisr.ttf", "filename of the ttf font")
s/\*string //

http://codereview.appspot.com/940044/diff/14001/15003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/14001/15003#newcode21
freetype/truetype/truetype.go:21: // An Index is a Font's index of a Unicode
character.
s/character/code point/ ?  i'm actually not sure

http://codereview.appspot.com/940044/diff/14001/15003#newcode25
freetype/truetype/truetype.go:25: // The endpoints are inclusive.
really? strange decision.

http://codereview.appspot.com/940044/diff/14001/15003#newcode163
freetype/truetype/truetype.go:163: readU16Slice(f.cmapOffset,
data[16+6*segCount:])
this is better and fine if you really want it this way but i would have used a
few read methods on the slice. the offsets in this code could be handled
completely automatically.

similarly with almost all the other slice offset stuff in this program.

[nigeltao, 2010-04-30 01:12:09]

http://codereview.appspot.com/940044/diff/14001/15001
File example/truetype/main.go (right):

http://codereview.appspot.com/940044/diff/14001/15001#newcode17
example/truetype/main.go:17: var fontfile *string = flag.String("fontfile",
"../../luxi-fonts/luxisr.ttf", "filename of the ttf font")
On 2010/04/30 00:15:17, r wrote:
> s/\*string //

Done.

http://codereview.appspot.com/940044/diff/14001/15003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/14001/15003#newcode21
freetype/truetype/truetype.go:21: // An Index is a Font's index of a Unicode
character.
On 2010/04/30 00:15:17, r wrote:
> s/character/code point/ ?  i'm actually not sure

I've gone with "code point".

http://codereview.appspot.com/940044/diff/14001/15003#newcode163
freetype/truetype/truetype.go:163: readU16Slice(f.cmapOffset,
data[16+6*segCount:])
On 2010/04/30 00:15:17, r wrote:
> this is better and fine if you really want it this way but i would have used a
> few read methods on the slice. the offsets in this code could be handled
> completely automatically.
> 
> similarly with almost all the other slice offset stuff in this program.

I'll leave it as is. If I get the itch I might try it your way and send out a
change if it looks better.

[nigeltao_gnome, 2010-05-02 07:33:16]

Ping.

[rsc1, 2010-05-02 08:08:39]

LGTM

http://codereview.appspot.com/940044/diff/20001/21003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/20001/21003#newcode92
freetype/truetype/truetype.go:92: cmapStart, cmapEnd, cmapDelta, cmapOffset
[]uint16
Isn't this really an []cmap where

type cmap struct {
   start, end, delta, offset uint16
}

?

http://codereview.appspot.com/940044/diff/20001/21003#newcode260
freetype/truetype/truetype.go:260: for i := 0; i < n; i++ {
After the above, you can use a range for this.

http://codereview.appspot.com/940044/diff/20001/21003#newcode264
freetype/truetype/truetype.go:264: } else {
No else after return

http://codereview.appspot.com/940044/diff/20001/21003#newcode278
freetype/truetype/truetype.go:278: } else if j >= f.nHMetric {
no else after return

http://codereview.appspot.com/940044/diff/20001/21003#newcode353
freetype/truetype/truetype.go:353: err = f.parseHead()
could tighten these up with

if err = f.parseHead; err != nil {
    return
}
...

[r, 2010-05-02 15:38:43]

LGTM but i still think you should try my suggestion with methods on a slice type
for the unpacking.

[nigeltao, 2010-05-03 14:42:34]

On 2010/05/02 15:38:43, r wrote:
> LGTM but i still think you should try my suggestion with methods on a slice
type
> for the unpacking.

Done. PTAL.

(Russ' comments are also all Done.)

[r, 2010-05-03 18:12:05]

LGTM

[rsc1, 2010-05-03 18:16:37]

LGTM

http://codereview.appspot.com/940044/diff/28001/29003
File freetype/truetype/truetype.go (right):

http://codereview.appspot.com/940044/diff/28001/29003#newcode52
freetype/truetype/truetype.go:52: func (d *data) u32() uint32 {
This is fine, but calling the method uint32 is more typical.
etc.

[nigeltao, 2010-05-03 23:57:40]

*** Submitted as
http://code.google.com/p/freetype-go/source/detail?r=19783ba3c8e0 ***

Freetype-Go: truetype parser initial check-in.

Parsing a malformed TTF may result in an index out-of-bounds panic,
rather than guaranteeing to return an error. This may be tightened
in the future.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/940044