code review 4630056: gofix: fixes for os/signal changes

gofix: fixes for os/signal changes

Fixes issue 1971.

[rh, 2011-06-22 00:37:50]

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

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

[adg, 2011-06-22 00:55:52]

Nice work.

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal_test.go
File src/cmd/gofix/signal_test.go (right):

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal_test.go...
src/cmd/gofix/signal_test.go:11: var signalTests = []testCase{
Perhaps add one more test to verify keepSignalImport works.

[adg, 2011-06-22 01:04:44]

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode425
src/cmd/gofix/fix.go:425: func addImport(f *ast.File, path string) {
It would be nice if this would add the import in sorted order.

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode450
src/cmd/gofix/fix.go:450: if ok && gen.Tok == token.IMPORT {
if !ok || gen.Tok != token.IMPORT {
    continue
}

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode454
src/cmd/gofix/fix.go:454: if err == nil && t == path {
if err != nil || t != path {
    continue
}

[rh, 2011-06-22 02:31:05]

Thanks for the review!  PTAL.

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode425
src/cmd/gofix/fix.go:425: func addImport(f *ast.File, path string) {
On 2011/06/22 01:04:45, adg wrote:
> It would be nice if this would add the import in sorted order.

addImport and removeImport are a bit painful.

The new version tries to respect alphabetically-sorted imports in the style of:

import (
    "a"
    "b"
)

For other styles of imports, it produces legal, ugly code. :)

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode450
src/cmd/gofix/fix.go:450: if ok && gen.Tok == token.IMPORT {
On 2011/06/22 01:04:45, adg wrote:
> if !ok || gen.Tok != token.IMPORT {
>     continue
> }

Done.

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/fix.go#newcode454
src/cmd/gofix/fix.go:454: if err == nil && t == path {
On 2011/06/22 01:04:45, adg wrote:
> if err != nil || t != path {
>     continue
> }

Done.

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal.go
File src/cmd/gofix/signal.go (right):

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal.go#newc...
src/cmd/gofix/signal.go:32: if isTopName(n.X, "signal") {
This assumes that "os" and "os/signal" are not imported under different package
names.  Is that alright?

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal_test.go
File src/cmd/gofix/signal_test.go (right):

http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal_test.go...
src/cmd/gofix/signal_test.go:11: var signalTests = []testCase{
On 2011/06/22 00:55:52, adg wrote:
> Perhaps add one more test to verify keepSignalImport works.

Done, and tested the new alphabetical import stuff too.

[adg, 2011-06-22 03:16:07]

On 22 June 2011 12:31,  <robert.hencke@gmail.com> wrote:
>
http://codereview.appspot.com/4630056/diff/10001/src/cmd/gofix/signal.go#newc...
> src/cmd/gofix/signal.go:32: if isTopName(n.X, "signal") {
> This assumes that "os" and "os/signal" are not imported under different
> package names.  Is that alright?

That seems reasonable.

Andrew

[rh, 2011-06-22 03:50:17]

OK, thank you.  PTAL.

I went ahead and fixed filepath, too.  If you'd prefer it in a separate CL,
though, please let me know.

[adg, 2011-06-22 04:03:43]

On 2011/06/22 03:50:17, rh wrote:
> OK, thank you.  PTAL.
> 
> I went ahead and fixed filepath, too.  If you'd prefer it in a separate CL,
> though, please let me know.

separate CL, please.

[rh, 2011-06-22 04:15:38]

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

Please take another look.

[adg, 2011-06-22 05:38:42]

LGTM

Leaving for rsc.

[rsc, 2011-06-22 20:04:49]

Very nice.

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/Makefile
File src/cmd/gofix/Makefile (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/Makefile#newcode19
src/cmd/gofix/Makefile:19: signal.go\
up one line (sort)

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go#newcode424
src/cmd/gofix/fix.go:424: // addImport adds the import path to the file f.
Wow, thanks for adding this.
I believe the algorithm should be slightly different:

* if imports(f, path) { return }
* Always add to the first import.
* Change non-parenthesized to parenthesized if needed.
* Add alphabetically.

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go#newcode474
src/cmd/gofix/fix.go:474: // deleteImport deletes the import path from the file
f, if present.
Thanks for adding this too.  How do we know when it's safe to call?
Should there be a usesImport(*ast.File, string) too?

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/signal.go
File src/cmd/gofix/signal.go (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/signal.go#newco...
src/cmd/gofix/signal.go:47: if !imports(f, "os") {
With the changes I suggested this can simplify to

addImport(f, "os")
if !usesImport(f, "os/signal") {
    deleteImport(f, "os/signal")
}

(without the keepSignalImport bool).
In this case maintaining keepSignalImport is easy
but in general it could be a pain to watch for uses.

[rh, 2011-06-24 02:48:05]

Thank you!  PTAL

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/Makefile
File src/cmd/gofix/Makefile (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/Makefile#newcode19
src/cmd/gofix/Makefile:19: signal.go\
On 2011/06/22 20:04:49, rsc wrote:
> up one line (sort)

Done.

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go#newcode424
src/cmd/gofix/fix.go:424: // addImport adds the import path to the file f.
On 2011/06/22 20:04:49, rsc wrote:
> Wow, thanks for adding this.
> I believe the algorithm should be slightly different:
> 
> * if imports(f, path) { return }
> * Always add to the first import.
> * Change non-parenthesized to parenthesized if need
> * Add alphabetically.
> 

Thanks!
Done, but I'd appreciate feedback on the implementation... it feels evil.

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/fix.go#newcode474
src/cmd/gofix/fix.go:474: // deleteImport deletes the import path from the file
f, if present.
On 2011/06/22 20:04:49, rsc wrote:
> Thanks for adding this too.  How do we know when it's safe to call?
> Should there be a usesImport(*ast.File, string) too?

Done.

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/signal.go
File src/cmd/gofix/signal.go (right):

http://codereview.appspot.com/4630056/diff/9006/src/cmd/gofix/signal.go#newco...
src/cmd/gofix/signal.go:47: if !imports(f, "os") {
On 2011/06/22 20:04:49, rsc wrote:
> With the changes I suggested this can simplify to
> 
> addImport(f, "os")
> if !usesImport(f, "os/signal") {
>     deleteImport(f, "os/signal")
> }
> 
> (without the keepSignalImport bool).
> In this case maintaining keepSignalImport is easy
> but in general it could be a pain to watch for uses.

Done.

[rsc, 2011-06-27 15:33:00]

looks pretty good; a few minor things below

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode265
src/cmd/gofix/fix.go:265: // getImport returns the import spec if f imports
path,
http://golang.org/doc/effective_go.html#Getters

I'd call these importSpec and importPath.

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode480
src/cmd/gofix/fix.go:480: impdecl.Lparen = -1
Seems like this can be

if len(impdecl.Specs) > 0 && !impdecl.Lparen.IsValid() {
    impdecl.Lparen = impdecl.Pos()
}

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode528
src/cmd/gofix/fix.go:528: }
else if len(gen.Specs) == 1 {
    gen.Lparen = token.NoPos  // drop parens
}

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode544
src/cmd/gofix/fix.go:544: defer func() {
delete

[rh, 2011-06-28 03:36:32]

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go
File src/cmd/gofix/fix.go (right):

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode265
src/cmd/gofix/fix.go:265: // getImport returns the import spec if f imports
path,
On 2011/06/27 15:33:00, rsc wrote:
> http://golang.org/doc/effective_go.html#Getters
> 
> I'd call these importSpec and importPath.

Old habits die hard... Done.

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode480
src/cmd/gofix/fix.go:480: impdecl.Lparen = -1
On 2011/06/27 15:33:00, rsc wrote:
> Seems like this can be
> 
> if len(impdecl.Specs) > 0 && !impdecl.Lparen.IsValid() {
>     impdecl.Lparen = impdecl.Pos()
> }

Done.  Thanks.  That's much better.

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode528
src/cmd/gofix/fix.go:528: }
On 2011/06/27 15:33:00, rsc wrote:
> else if len(gen.Specs) == 1 {
>     gen.Lparen = token.NoPos  // drop parens
> }

Done.

http://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode544
src/cmd/gofix/fix.go:544: defer func() {
On 2011/06/27 15:33:00, rsc wrote:
> delete

Done.

[rh, 2011-06-28 03:36:46]

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

Please take another look.

[adg, 2011-06-29 06:25:31]

LGTM

[adg, 2011-06-29 06:32:56]

Actually, no it doesn't.

The tests don't pass.

	signal.0: incorrect output.
	--- have
	package main
	
	import (
		_ "a"
		"os/signal"
		_ "z"
	)
	
	type T1 signal.UnixSignal
	type T2 signal.Signal
	
	func f() {
		_ = signal.SIGHUP
		_ = signal.Incoming
	}
	
	--- want
	package main
	
	import (
		_ "a"
		"os"
		"os/signal"
		_ "z"
	)
	
	type T1 os.UnixSignal
	type T2 os.Signal
	
	func f() {
		_ = os.SIGHUP
		_ = signal.Incoming
	}


	signal.1: incorrect output.
	--- have
	package main
	
	import (
		"os"
		"os/signal"
	)
	
	func f() {
		var _ os.Error
		_ = signal.SIGHUP
	}
	
	--- want
	package main
	
	import "os"
	
	
	func f() {
		var _ os.Error
		_ = os.SIGHUP
	}

	signal.2: incorrect output.
	--- have
	package main
	
	import "os"
	import "os/signal"
	
	func f() {
		var _ os.Error
		_ = signal.SIGHUP
	}
	
	--- want
	package main
	
	import "os"
	
	
	func f() {
		var _ os.Error
		_ = os.SIGHUP
	}

[rh, 2011-06-29 06:39:04]

That's odd.  It passes locally for me.  Let me make sure I uploaded
everything...

On 2011/06/29 06:32:56, adg wrote:
> Actually, no it doesn't.
> 
> The tests don't pass.
> 
> 	signal.0: incorrect output.
> 	--- have
> 	package main
> 	
> 	import (
> 		_ "a"
> 		"os/signal"
> 		_ "z"
> 	)
> 	
> 	type T1 signal.UnixSignal
> 	type T2 signal.Signal
> 	
> 	func f() {
> 		_ = signal.SIGHUP
> 		_ = signal.Incoming
> 	}
> 	
> 	--- want
> 	package main
> 	
> 	import (
> 		_ "a"
> 		"os"
> 		"os/signal"
> 		_ "z"
> 	)
> 	
> 	type T1 os.UnixSignal
> 	type T2 os.Signal
> 	
> 	func f() {
> 		_ = os.SIGHUP
> 		_ = signal.Incoming
> 	}
> 
> 
> 	signal.1: incorrect output.
> 	--- have
> 	package main
> 	
> 	import (
> 		"os"
> 		"os/signal"
> 	)
> 	
> 	func f() {
> 		var _ os.Error
> 		_ = signal.SIGHUP
> 	}
> 	
> 	--- want
> 	package main
> 	
> 	import "os"
> 	
> 	
> 	func f() {
> 		var _ os.Error
> 		_ = os.SIGHUP
> 	}
> 
> 	signal.2: incorrect output.
> 	--- have
> 	package main
> 	
> 	import "os"
> 	import "os/signal"
> 	
> 	func f() {
> 		var _ os.Error
> 		_ = signal.SIGHUP
> 	}
> 	
> 	--- want
> 	package main
> 	
> 	import "os"
> 	
> 	
> 	func f() {
> 		var _ os.Error
> 		_ = os.SIGHUP
> 	}

[rh, 2011-06-29 06:41:54]

ah dangit.

The Makefile got removed from the CL somehow.

PTAL.

[adg, 2011-06-29 06:44:36]

LGTM

Glad it was that simple. :-)

[adg, 2011-06-29 06:44:56]

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

gofix: fixes for os/signal changes

Fixes issue 1971.

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/4630056

Committer: Andrew Gerrand <adg@golang.org>