code review 4849049: exp/template: split the parse tree into a separate pack...

exp/template: split the parse tree into a separate package exp/template/parse

Mostly a mechanical change, with a few cleanups to make the split easier.
The external interface to exp/template is unaffected.

In another round I will play with the function map setup to see if I can
avoid exposing reflect across the boundary, but that will require some
structural changes I did not want to mix into this CL.

[r, 2011-08-09 03:38:09]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-08-09 03:52:50]

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/exec.go#new...
src/pkg/exp/template/exec.go:87: return
useless return

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/funcs.go
File src/pkg/exp/template/funcs.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/funcs.go#ne...
src/pkg/exp/template/funcs.go:30: var builtins = map[string]reflect.Value{
how come you don't just want to export this var?

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/parse.go
File src/pkg/exp/template/parse.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/parse.go#ne...
src/pkg/exp/template/parse.go:49: if err != nil {
could you replace these four lines with just this?
  return t, err

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/set.go
File src/pkg/exp/template/set.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/set.go#newc...
src/pkg/exp/template/set.go:104: return s, err
err is always nil here

[r, 2011-08-09 04:01:38]

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/exec.go#new...
src/pkg/exp/template/exec.go:87: return
On 2011/08/09 03:52:50, dsymonds wrote:
> useless return

Done.

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/funcs.go
File src/pkg/exp/template/funcs.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/funcs.go#ne...
src/pkg/exp/template/funcs.go:30: var builtins = map[string]reflect.Value{
On 2011/08/09 03:52:50, dsymonds wrote:
> how come you don't just want to export this var?

it's a piece of my thinking about how to manage the function maps.
if it's a function rather than just a variable, it'll help discourage people
modifying it, i hope.

i only exported it because i thought it might be useful, but i don't need
it to be exported now, so i'll undo that.

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/parse.go
File src/pkg/exp/template/parse.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/parse.go#ne...
src/pkg/exp/template/parse.go:49: if err != nil {
On 2011/08/09 03:52:50, dsymonds wrote:
> could you replace these four lines with just this?
>   return t, err

then t won't be nil if there's an error, which is a recipe for bugs

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/set.go
File src/pkg/exp/template/set.go (right):

http://codereview.appspot.com/4849049/diff/1/src/pkg/exp/template/set.go#newc...
src/pkg/exp/template/set.go:104: return s, err
On 2011/08/09 03:52:50, dsymonds wrote:
> err is always nil here

Done.

[r, 2011-08-09 04:01:49]

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

Please take another look.

[dsymonds, 2011-08-09 04:10:20]

LGTM

[r, 2011-08-09 05:43:08]

*** Submitted as http://code.google.com/p/go/source/detail?r=14872cc7275f ***

exp/template: split the parse tree into a separate package exp/template/parse

Mostly a mechanical change, with a few cleanups to make the split easier.
The external interface to exp/template is unaffected.

In another round I will play with the function map setup to see if I can
avoid exposing reflect across the boundary, but that will require some
structural changes I did not want to mix into this CL.

R=golang-dev, dsymonds
CC=golang-dev
http://codereview.appspot.com/4849049