code review 6347057: freetype/truetype: first cut of a bytecode interpreter.

freetype/truetype: first cut of a bytecode interpreter.

[nigeltao, 2012-07-02 13:01:37]

Hello gri@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/freetype-go

[bsiegert, 2012-07-02 13:16:43]

Looks nice, just two style remarks. Was this translated from C code or written
from scratch?

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go
File freetype/truetype/hint.go (right):

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:67: // SWAP[] SWAP the top two elements on the stack
Use multiple assignment.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:207: goto pushb
Why not put the pushb code here directly?

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:212: goto pushw
ditto for pushw

[gri, 2012-07-02 20:22:21]

LGTM

But please address the comments before submitting. In particular, I think there
should be named constants for the opcodes.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go
File freetype/truetype/hint.go (right):

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:24: pc, top   int32
can't these just be int?

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:25: loopCount int
instrCount? (it's not counting loops), or perhaps just: "steps" ?

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:26: // x and y are general purpose int32 variables.
I think it's only worthwhile doing this here (x, y, x64, y64) if we can be
fairly certain that they are allocated in registers. Otherwise I would declare
them in the individual cases because they will always be initialized right in
the declaration and it's very likely that they are going to be in registers.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:31: for int(pc) < len(program) {
if pc is int, this cast is not needed

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:34: return errors.New("truetype: hinting: too many
instructions")
I would probably introduce a helper function that adds the "truetype: hinting"
prefix. E.g.:

func err(msg string) error

and then just:

return err("too many instructions")

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:46: case 0x20:
Any reason for not defining an "enum" for the opcodes? then the comments
wouldn't have to repeat the name.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:54: pc++
How about factoring this out, say at the end of the switch? Control-flow would
use continue to not execute it. Alternatively, do it always and correct in
control-flow by subtracting one?

There seem to be only a small number of cases where you add extra code this way
(e.g. NPUSHB you can increment the pc early). Not sure it's worth doing it
explicitly in each instruction.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:110: case 0x50, 0x51, 0x52, 0x53, 0x54, 0x55:
These I would split out into separate instructions, even though you will need to
duplicate some code (assuming they are frequently used instructions). I would
try to reduce the amount of extra control flow (switch):

case 0x50:
   h.stack[top-2] = bool2int32(h.stack[top-2] < h.stack[top-1])
   top--

is not that bad (with pc++ factored out)

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:117: x = h.stack[top-1]
If you want to leave the code as is, at least swap x and y here so that below
the code reads x < y rather y < x.

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:138: // AND[] logical AND
again, I'd split this into two cases

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:145: x = bool2int32(x != 0 || y != 0)
x|y != 0

(less control flow - not sure it's worthwhile)

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:157: case 0x60, 0x61:
split into two cases

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:169: case 0x62, 0x63:
split into two cases

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:185: case 0x64, 0x65:
ditto

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:194: case 0x66, 0x67:
ditto

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:207: goto pushb
On 2012/07/02 13:16:43, bsiegert wrote:
> Why not put the pushb code here directly?
because it's used from other places as well

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:212: goto pushw
On 2012/07/02 13:16:43, bsiegert wrote:
> ditto for pushw
ditto

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:213: }
default case? I assume eventually you want to get rid of the popCount[opcode] ==
q test in the beginning

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:252: var popCount = [256]uint8{
don't know if you plan to keep this table around, but if so, please add numbers
0, 1, 2, ...C, D, E, F as comments left and at the top so that one can easily
find an entry

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint_test.go
File freetype/truetype/hint_test.go (right):

http://codereview.appspot.com/6347057/diff/5001/freetype/truetype/hint_test.g...
freetype/truetype/hint_test.go:32: 0xb2, // PUSHB[010] -> [10, 20, 30]
if you use named constants for the opcodes, the comments don't need to repeat
the opcode name

[nigeltao, 2012-07-03 10:45:50]

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

freetype/truetype: first cut of a bytecode interpreter.

R=gri, bsiegert
CC=golang-dev
http://codereview.appspot.com/6347057