code review 106010046: go/parser: extra test cases for ParseExpr

go/parser: don't accept trailing explicit semicolon

Fixes issue 8207.

[gri, 2014-06-14 19:21:32]

Hello golang-codereviews@googlegroups.com,

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

[wandakkelly_gmail.com, 2014-06-15 04:32:08]

https://groups.google.com/forum/?hl=th#!forum/pump-heels

เมื่อ วันอาทิตย์ที่ 15 มิถุนายน ค.ศ. 2014, 2 นาฬิกา 21 นาที 32 วินาที 
UTC+7, g...@golang.org เขียนว่า:
>
> Reviewers: golang-codereviews, 
>
> Message: 
> Hello golang-co...@googlegroups.com <javascript:>, 
>
> I'd like you to review this change to 
> https://code.google.com/p/go 
>
>
> Description: 
> go/parser: extra test cases for ParseExpr 
>
> Per issue 8207. 
>
> Please review this at https://codereview.appspot.com/106010046/ 
>
> Affected files (+11, -4 lines): 
>    M src/pkg/go/parser/parser_test.go 
>
>
> Index: src/pkg/go/parser/parser_test.go 
> =================================================================== 
> --- a/src/pkg/go/parser/parser_test.go 
> +++ b/src/pkg/go/parser/parser_test.go 
> @@ -94,18 +94,25 @@ 
>
>           // an invalid expression 
>           src = "a + *" 
> -        _, err = ParseExpr(src) 
> -        if err == nil { 
> +        if _, err := ParseExpr(src); err == nil { 
>                   t.Fatalf("ParseExpr(%s): got no error", src) 
>           } 
>
>           // a valid expression followed by extra tokens is invalid 
>           src = "a[i] := x" 
> -        _, err = ParseExpr(src) 
> -        if err == nil { 
> +        if _, err := ParseExpr(src); err == nil { 
>                   t.Fatalf("ParseExpr(%s): got no error", src) 
>           } 
>
> +        const validExpr = "a + b" 
> +        const anything = "dh3*#D)#_" 
> +        for _, c := range "!)]};," { 
> +                src := validExpr + string(c) + anything 
> +                if _, err := ParseExpr(src); err == nil { 
> +                        t.Fatalf("ParseExpr(%s): got no error", src) 
> +                } 
> +        } 
> + 
>           // ParseExpr must not crash 
>           for _, src := range valids { 
>                   ParseExpr(src) 
>
>
>

[gmk, 2014-06-15 07:52:14]

On 2014/06/14 19:21:32, gri wrote:
> Hello mailto:golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go

LGTM

[bradfitz, 2014-06-15 19:16:51]

LGTM

I'd use %q in the Fatalf lines, though.  The closing paren in ParseExpr(a +
b)) won't be easy to see.



On Sat, Jun 14, 2014 at 12:21 PM, <gri@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> go/parser: extra test cases for ParseExpr
>
> Per issue 8207.
>
> Please review this at https://codereview.appspot.com/106010046/
>
> Affected files (+11, -4 lines):
>   M src/pkg/go/parser/parser_test.go
>
>
> Index: src/pkg/go/parser/parser_test.go
> ===================================================================
> --- a/src/pkg/go/parser/parser_test.go
> +++ b/src/pkg/go/parser/parser_test.go
> @@ -94,18 +94,25 @@
>
>         // an invalid expression
>         src = "a + *"
> -       _, err = ParseExpr(src)
> -       if err == nil {
> +       if _, err := ParseExpr(src); err == nil {
>                 t.Fatalf("ParseExpr(%s): got no error", src)
>         }
>
>         // a valid expression followed by extra tokens is invalid
>         src = "a[i] := x"
> -       _, err = ParseExpr(src)
> -       if err == nil {
> +       if _, err := ParseExpr(src); err == nil {
>                 t.Fatalf("ParseExpr(%s): got no error", src)
>         }
>
> +       const validExpr = "a + b"
> +       const anything = "dh3*#D)#_"
> +       for _, c := range "!)]};," {
> +               src := validExpr + string(c) + anything
> +               if _, err := ParseExpr(src); err == nil {
> +                       t.Fatalf("ParseExpr(%s): got no error", src)
> +               }
> +       }
> +
>         // ParseExpr must not crash
>         for _, src := range valids {
>                 ParseExpr(src)
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[gri, 2014-06-16 23:27:41]

Hello golang-codereviews@googlegroups.com, wandakkelly@gmail.com,
gordon.klaus@gmail.com, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[gmk, 2014-06-17 06:33:00]

LGTM

https://codereview.appspot.com/106010046/diff/60001/src/pkg/go/parser/parser_...
File src/pkg/go/parser/parser_test.go (right):

https://codereview.appspot.com/106010046/diff/60001/src/pkg/go/parser/parser_...
src/pkg/go/parser/parser_test.go:77: t.Fatalf("ParseExpr(%q): %v", src, err)
Did you want to also change this to Errorf like the others below?

[gri, 2014-06-17 15:58:10]

https://codereview.appspot.com/106010046/diff/60001/src/pkg/go/parser/parser_...
File src/pkg/go/parser/parser_test.go (right):

https://codereview.appspot.com/106010046/diff/60001/src/pkg/go/parser/parser_...
src/pkg/go/parser/parser_test.go:77: t.Fatalf("ParseExpr(%q): %v", src, err)
On 2014/06/17 06:32:59, gmk wrote:
> Did you want to also change this to Errorf like the others below?

Done.

[gri, 2014-06-17 15:58:11]

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

go/parser: don't accept trailing explicit semicolon

Fixes issue 8207.

LGTM=gordon.klaus, bradfitz
R=golang-codereviews, wandakkelly, gordon.klaus, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/106010046