code review 13283045: cmd/yacc: replace units example with simpler expr example

cmd/yacc: replace units example with simpler expr example

The units example is nice but is covered by the Lucent
license, which may be a concern for some people making a
commercial source code distribution of Go.

[iant, 2013-09-11 03:36:59]

Hello golang-dev@googlegroups.com,

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

[r, 2013-09-11 04:03:28]

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y
File src/cmd/yacc/expr.y (right):

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode15
src/cmd/yacc/expr.y:15: // file, so it won't confuse a future build.
// This tag will be copied to the generated file to prevent that file confusing
a future build.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode117
src/cmd/yacc/expr.y:117: return '*'
worth a comment

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode122
src/cmd/yacc/expr.y:122: log.Printf("unrecognized character '%c'", c)
s/'%c'/%q/

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode139
src/cmd/yacc/expr.y:139: case 0:
why? the default catches it.
would be clearer if 0 had a name

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode153
src/cmd/yacc/expr.y:153: log.Printf("bad number %s", b.String())
s/%s/%q/ ?

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode167
src/cmd/yacc/expr.y:167: return 0
s/0/something/

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode171
src/cmd/yacc/expr.y:171: if c == utf8.RuneError {
if c== utf8.RuneError && size==1

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode172
src/cmd/yacc/expr.y:172: log.Printf("invalid utf8")
Print or Println; no need for f.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode189
src/cmd/yacc/expr.y:189: line, err := in.ReadBytes('\n')
use bufio.Scan instead

[iant, 2013-09-11 04:22:36]

PTAL

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y
File src/cmd/yacc/expr.y (right):

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode15
src/cmd/yacc/expr.y:15: // file, so it won't confuse a future build.
On 2013/09/11 04:03:29, r wrote:
> // This tag will be copied to the generated file to prevent that file
confusing
> a future build.

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode117
src/cmd/yacc/expr.y:117: return '*'
On 2013/09/11 04:03:29, r wrote:
> worth a comment

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode122
src/cmd/yacc/expr.y:122: log.Printf("unrecognized character '%c'", c)
On 2013/09/11 04:03:29, r wrote:
> s/'%c'/%q/

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode139
src/cmd/yacc/expr.y:139: case 0:
On 2013/09/11 04:03:29, r wrote:
> why? the default catches it.
> would be clearer if 0 had a name

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode153
src/cmd/yacc/expr.y:153: log.Printf("bad number %s", b.String())
On 2013/09/11 04:03:29, r wrote:
> s/%s/%q/ ?

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode167
src/cmd/yacc/expr.y:167: return 0
On 2013/09/11 04:03:29, r wrote:
> s/0/something/

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode171
src/cmd/yacc/expr.y:171: if c == utf8.RuneError {
On 2013/09/11 04:03:29, r wrote:
> if c== utf8.RuneError && size==1

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode172
src/cmd/yacc/expr.y:172: log.Printf("invalid utf8")
On 2013/09/11 04:03:29, r wrote:
> Print or Println; no need for f.

Done.

https://codereview.appspot.com/13283045/diff/1/src/cmd/yacc/expr.y#newcode189
src/cmd/yacc/expr.y:189: line, err := in.ReadBytes('\n')
On 2013/09/11 04:03:29, r wrote:
> use bufio.Scan instead

It's not clear that I can do that and still reliably preserve the "> " prompt. 
The scan docs say they can read arbitrarily far ahead.  I need to only read up
to the newline.

[r, 2013-09-11 04:38:18]

LGTM

https://codereview.appspot.com/13283045/diff/6001/src/cmd/yacc/expr.y
File src/cmd/yacc/expr.y (right):

https://codereview.appspot.com/13283045/diff/6001/src/cmd/yacc/expr.y#newcode190
src/cmd/yacc/expr.y:190: in := bufio.NewReader(os.Stdin)
i'm nearly certain Scan would work here, but i won't press the point.

[iant, 2013-09-11 16:01:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=a74e4a7820d0 ***

cmd/yacc: replace units example with simpler expr example

The units example is nice but is covered by the Lucent
license, which may be a concern for some people making a
commercial source code distribution of Go.

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/13283045

[rog, 2013-09-12 08:36:08]

https://codereview.appspot.com/13283045/diff/13001/src/cmd/yacc/expr.y
File src/cmd/yacc/expr.y (right):

https://codereview.appspot.com/13283045/diff/13001/src/cmd/yacc/expr.y#newcode58
src/cmd/yacc/expr.y:58: $$ = $2
One thought - it might be nice to use $expr rather than $2, as it's an unusual
and nice feature that this particular yacc provides.

(And below too).

[iant, 2013-09-12 16:00:29]

On Thu, Sep 12, 2013 at 1:36 AM,  <rogpeppe@gmail.com> wrote:
>
> https://codereview.appspot.com/13283045/diff/13001/src/cmd/yacc/expr.y
> File src/cmd/yacc/expr.y (right):
>
>
https://codereview.appspot.com/13283045/diff/13001/src/cmd/yacc/expr.y#newcode58
> src/cmd/yacc/expr.y:58: $$ = $2
> One thought - it might be nice to use $expr rather than $2, as it's an
> unusual and nice feature that this particular yacc provides.
>
> (And below too).

Sure, want to send the change?

Ian