code review 6354067: freetype/truetype: if/else opcodes.

freetype/truetype: if/else opcodes.

[nigeltao, 2012-07-03 12:26:46]

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

[gri, 2012-07-03 17:02:36]

LGTM

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

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:42: case opELSE:
I would have just chosen the full capitalized name for the opcodes and not
worried about them being exported. They are "global" anyway. I think it would
read more nicely and less cluttered. So:

case ELSE:
case POP:
case DUP:
etc.

(or perhaps prefix with_, as in _ELSE, _POP, if you must).

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:182: pc++
It might be interesting to see if the compiler generates different (better) code
if the pc++ is part of the for loop rather than have it here: Depending on how
the for loop is translated, if the pc++ is done there, it may save an extra
(unconditional) branch. Not sure if it matters in terms of performance, though.
You would have to correct by -1 in cases where you use continue now.In the old
days, that would have been preferred over the extra control flow. Not sure what
CPUs do nowadays.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:186: // Skip past bytecode until the next ELSE (if
opcode == 0) or the
wow, really? No direct branch in this instruction set?

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:220: case opPUSHB000, opPUSHB001, opPUSHB010,
opPUSHB011, opPUSHB100, opPUSHB101, opPUSHB110, opPUSHB111:
case PUSHB0, PUSHB1, ... PUSHB7: ?

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:222: case opPUSHW000, opPUSHW001, opPUSHW010,
opPUSHW011, opPUSHW100, opPUSHW101, opPUSHW110, opPUSHW111:
ditto

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go
File freetype/truetype/opcodes.go (right):

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go#...
freetype/truetype/opcodes.go:13: opSVTCA0    = 0x00
I understand the reason for the explicit constants, but you might want to
consider using iota - and simply group the opcodes in sets of 8 or 16. It might
still be easy to read:

const (
   SVTCA0 = 0x00 + iota
   SVTCA1
   SPVTCA0
   SPVTCA1
   SFVTCA0
   SFVTCA1
   SPVTL0
   SPVTL1
   ...

   opSRP0 = 0x10 + iota
   opSRP1
   ...
)

By repeating the iota with respective offset you avoid getting off-by-one errors
through the entire opcode set. An alternative grouping might be by function.

Just a thought.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go#...
freetype/truetype/opcodes.go:156: op_0x8f     = 0x8f
leave this away if they are not used?

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go#...
freetype/truetype/opcodes.go:273: // 1, 2, 3, 4, 5, 6, 7, 8, 9, a, b, c, d, e, f
I see - gofmt is not your friend in this case ... :-)

[nigeltao, 2012-07-04 11:17:08]

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

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:42: case opELSE:
On 2012/07/03 17:02:36, gri wrote:
> I would have just chosen the full capitalized name for the opcodes and not
> worried about them being exported. They are "global" anyway. I think it would
> read more nicely and less cluttered.

We could want to export these constants if we expect people to want to write
tools in Go that write Truetype hinting programs. But I don't expect that to
happen anytime soon, and maybe less so in the brave new world of 300dpi screens.
If it did, I'd probably want some functions to assemble and disassemble opcodes,
which probably means moving these constants to a separate truetype/bytecode
package.

In the meantime, I'd rather not clutter up these package docs with 200 opcode
constants. I also prefer opELSE over _ELSE.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:182: pc++
On 2012/07/03 17:02:36, gri wrote:
> It might be interesting to see if the compiler generates different (better)
code
> if the pc++ is part of the for loop rather than have it here: Depending on how
> the for loop is translated, if the pc++ is done there, it may save an extra
> (unconditional) branch. Not sure if it matters in terms of performance,
though.
> You would have to correct by -1 in cases where you use continue now.In the old
> days, that would have been preferred over the extra control flow. Not sure
what
> CPUs do nowadays.

It might be a little faster, but the -1 corrections seem artificial. I don't
think performance matters. I would expect most fonts to only execute tens of
instructions per glyph, and the resultant hinted glyph is then cached.

If it matters, we can optimize it once we have a full test suite.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:186: // Skip past bytecode until the next ELSE (if
opcode == 0) or the
On 2012/07/03 17:02:36, gri wrote:
> wow, really? No direct branch in this instruction set?

There's relative jumps (JMPR, JROF, JROT), but yeah, the IF/ELSEs need a
bytecode walk.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/hint.go#new...
freetype/truetype/hint.go:220: case opPUSHB000, opPUSHB001, opPUSHB010,
opPUSHB011, opPUSHB100, opPUSHB101, opPUSHB110, opPUSHB111:
On 2012/07/03 17:02:36, gri wrote:
> case PUSHB0, PUSHB1, ... PUSHB7: ?

I can see the appeal, but the spec at
https://developer.apple.com/fonts/TTRefMan/RM07/appendixA.html uses 000, 001,
etc.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go
File freetype/truetype/opcodes.go (right):

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go#...
freetype/truetype/opcodes.go:13: opSVTCA0    = 0x00
On 2012/07/03 17:02:36, gri wrote:
> I understand the reason for the explicit constants, but you might want to
> consider using iota - and simply group the opcodes in sets of 8 or 16.

I prefer explicit constants.

http://codereview.appspot.com/6354067/diff/3004/freetype/truetype/opcodes.go#...
freetype/truetype/opcodes.go:156: op_0x8f     = 0x8f
On 2012/07/03 17:02:36, gri wrote:
> leave this away if they are not used?

I prefer to be complete.

[nigeltao, 2012-07-04 11:28:57]

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

freetype/truetype: if/else opcodes.

R=gri
CC=golang-dev
http://codereview.appspot.com/6354067