code review 124700043: cmd/gofmt: fix error on partial Go code ending with lin...

cmd/gofmt: fix error on partial Go code ending with line comment.

Fix issue by always appending newline after user input, before
the closing curly bracket. The adjust func is modified to remove
this new newline.

Add test case (it fails before CL, passes after).

Fixes issue 8411.

[shurcooL, 2014-08-21 06:59:09]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-08-21 17:30:32]

R=gri



On Wed, Aug 20, 2014 at 11:59 PM, <shurcooL@gmail.com> 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:
> cmd/gofmt: fix error on partial Go code ending with line comment.
>
> Fix issue by always appending newline after user input, before
> the closing curly bracket. The adjust func is modified to remove
> this new newline.
>
> Add test case (it fails before CL, passes after).
>
> Fixes issue 8411.
>
> Please review this at https://codereview.appspot.com/124700043/
>
> Affected files (+4, -2 lines):
>   M src/cmd/gofmt/gofmt.go
>   A src/cmd/gofmt/testdata/stdin5.golden
>   A src/cmd/gofmt/testdata/stdin5.input
>
>
> Index: src/cmd/gofmt/gofmt.go
> ===================================================================
> --- a/src/cmd/gofmt/gofmt.go
> +++ b/src/cmd/gofmt/gofmt.go
> @@ -277,14 +277,14 @@
>         // into a function body.  This handles expressions too.
>         // Insert using a ;, not a newline, so that the line numbers
>         // in fsrc match the ones in src.
> -       fsrc := append(append([]byte("package p; func _() {"), src...),
> '}')
> +       fsrc := append(append([]byte("package p; func _() {"), src...),
> []byte("\n}")...)
>         file, err = parser.ParseFile(fset, filename, fsrc, parserMode)
>         if err == nil {
>                 adjust := func(orig, src []byte) []byte {
>                         // Remove the wrapping.
>                         // Gofmt has turned the ; into a \n\n.
>                         src = src[len("package p\n\nfunc _() {"):]
> -                       src = src[:len(src)-len("}\n")]
> +                       src = src[:len(src)-len("\n}\n")]
>                         // Gofmt has also indented the function body one
> level.
>                         // Remove that indent.
>                         src = bytes.Replace(src, []byte("\n\t"),
> []byte("\n"), -1)
> Index: src/cmd/gofmt/testdata/stdin5.golden
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/src/cmd/gofmt/testdata/stdin5.golden
> @@ -0,0 +1,1 @@
> +i := 5 // Line comment without newline.
> \ No newline at end of file
> Index: src/cmd/gofmt/testdata/stdin5.input
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/src/cmd/gofmt/testdata/stdin5.input
> @@ -0,0 +1,1 @@
> +i  :=5// Line comment without newline.
> \ No newline at end of file
>
>
> --
> 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.
>

[josharian, 2014-08-21 17:44:52]

R=gri

https://codereview.appspot.com/124700043/diff/40001/src/cmd/gofmt/gofmt.go
File src/cmd/gofmt/gofmt.go (right):

https://codereview.appspot.com/124700043/diff/40001/src/cmd/gofmt/gofmt.go#ne...
src/cmd/gofmt/gofmt.go:280: fsrc := append(append([]byte("package p; func _()
{"), src...), []byte("\n}")...)
No need to create a slice:

fsrc := append(append([]byte("package p; func _() {"), src...), '\n', '}')

[shurcooL, 2014-08-21 19:05:04]

Thanks, for the suggestion. I've considered both. After 45 seconds of thinking,
I decided to go with a string-representation rather than byte by byte in this
case because I thought it had slightly better clarity, and it scales better if
it needs to become 3, 4, 10 characters. Basically, I used a rule of thumb: "if
it's more than than 1 character, use string representation of byte slice".

I've benchmarked both approaches, and the difference in speed and allocations is
negligible. It's not inside a hot loop.

gofmt $ go test -v --bench=Rewrite --run=Nothing --test.benchmem
PASS
BenchmarkRewrite	     100	  16245624 ns/op	 2254396 B/op	   27798 allocs/op
gofmt $ go test -v --bench=Rewrite --run=Nothing --test.benchmem
PASS
BenchmarkRewrite	     100	  16120147 ns/op	 2249368 B/op	   27791 allocs/op

gofmt $ go test -v --bench=Fsrc --run=Nothing --test.benchmem
PASS
BenchmarkFsrc	 5000000	       394 ns/op	     320 B/op	       3 allocs/op
gofmt $ go test -v --bench=Fsrc --run=Nothing --test.benchmem
PASS
BenchmarkFsrc	 5000000	       435 ns/op	     328 B/op	       3 allocs/op

However, I have no problem changing it to:

    fsrc := append(append([]byte("package p; func _() {"), src...), '\n', '}')

If that is deemed better style in this case. It took me 45 seconds to decide
because both approaches are not very different from each other.


On 2014/08/21 17:44:52, josharian wrote:
> R=gri
> 
> https://codereview.appspot.com/124700043/diff/40001/src/cmd/gofmt/gofmt.go
> File src/cmd/gofmt/gofmt.go (right):
> 
>
https://codereview.appspot.com/124700043/diff/40001/src/cmd/gofmt/gofmt.go#ne...
> src/cmd/gofmt/gofmt.go:280: fsrc := append(append([]byte("package p; func _()
> {"), src...), []byte("\n}")...)
> No need to create a slice:
> 
> fsrc := append(append([]byte("package p; func _() {"), src...), '\n', '}')

[josharian, 2014-08-21 19:27:01]

> it scales better if it needs to become 3, 4, 10 characters

Build for the present.

Spelling out the two bytes is more efficient and just as readable.

But do what you will. Leaving for gri.

[gri, 2014-08-21 21:06:15]

I think the gofmt_test.go file needs to be adjusted for the new test to be
picked up.

[shurcooL, 2014-08-21 22:17:16]

The new stdin5 test is picked up without changes to gofmt_test.go.

The list of tests includes:

{"testdata/stdin*.input", "-stdin"},

Which is expanded to all five stdin* tests via filepath.Glob().

I also verified that by running go test without the change to
src/cmd/gofmt/gofmt.go,
and it failed. With the change to src/cmd/gofmt/gofmt.go it succeeded.


On 2014/08/21 21:06:15, gri wrote:
> I think the gofmt_test.go file needs to be adjusted for the new test to be
> picked up.

[gri, 2014-08-21 22:33:57]

Good point (I need to simplify that test framework). Please address
josharian's comment and re-upload. Thanks.
- gri


On Thu, Aug 21, 2014 at 3:17 PM, <shurcooL@gmail.com> wrote:

> The new stdin5 test is picked up without changes to gofmt_test.go.
>
> The list of tests includes:
>
> {"testdata/stdin*.input", "-stdin"},
>
> Which is expanded to all five stdin* tests via filepath.Glob().
>
> I also verified that by running go test without the change to
> src/cmd/gofmt/gofmt.go,
> and it failed. With the change to src/cmd/gofmt/gofmt.go it succeeded.
>
>
>
> On 2014/08/21 21:06:15, gri wrote:
>
>> I think the gofmt_test.go file needs to be adjusted for the new test
>>
> to be
>
>> picked up.
>>
>
>
> https://codereview.appspot.com/124700043/
>

[shurcooL, 2014-08-22 04:01:16]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org,
josharian@gmail.com, gri@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[gri, 2014-08-22 17:17:51]

LGTM

thanks

[gri, 2014-08-22 17:18:04]

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

cmd/gofmt: fix error on partial Go code ending with line comment.

Fix issue by always appending newline after user input, before
the closing curly bracket. The adjust func is modified to remove
this new newline.

Add test case (it fails before CL, passes after).

Fixes issue 8411.

LGTM=gri
R=golang-codereviews, bradfitz, josharian, gri
CC=golang-codereviews
https://codereview.appspot.com/124700043

Committer: Robert Griesemer <gri@golang.org>

[gobot, 2014-08-22 17:25:55]

This CL appears to have broken the freebsd-amd64-race builder.
See http://build.golang.org/log/ca53b562677bebbc01df42015607ef93cee2d27c