code review 4607052: os.Error API: don't export os.ErrorString, use os.NewEr...

os.Error API: don't export os.ErrorString, use os.NewError consistently

This is a core API change.

1) gofix misc src
2) Manual adjustments to the following files under src/pkg:
   gob/decode.go
   rpc/client.go
   os/error.go
   io/io.go
   bufio/bufio.go
   http/request.go
   websocket/client.go
as well as:
   src/cmd/gofix/testdata/*.go.in (reverted)
   test/fixedbugs/bug243.go
3) Implemented gofix patch (oserrorstring.go) and test case (oserrorstring_test.go)

Compiles and runs all tests.

[gri, 2011-06-21 00:38:27]

Hello golang-dev@googlegroups.com,

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

[r, 2011-06-21 01:21:16]

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.go
File src/cmd/gofix/oserrorstring.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:42: // Other occurences of os.ErrorString are not
fixed;
s/;//

http://codereview.appspot.com/4607052/diff/4081/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/bufio/bufio.go#newcode33
src/pkg/bufio/bufio.go:33: errInternal          os.Error =
&Error{os.NewError("bufio: internal error")}
helper function please. what is this, java?

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/persist.go
File src/pkg/http/persist.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:17: ErrPersistEOF =
&ProtocolError{os.NewError("persistent connection closed")}
helper function

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/request.go#newco...
src/pkg/http/request.go:46: ErrLineTooLong          =
&ProtocolError{os.NewError("header line too long")}
i'd write a helper function

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go#n...
src/pkg/websocket/client.go:26: ErrBadScheme            = os.NewError("bad
scheme") // TODO: shouldn't this be a ProtocolError?
fix TODO before submitting.

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go#n...
src/pkg/websocket/client.go:27: ErrBadStatus            =
&ProtocolError{os.NewError("bad status")}
helper function

[rsc, 2011-06-21 16:05:55]

Instead of using gofmt -r, please revert the other files
and run gofix to test that it can handle them.

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.go
File src/cmd/gofix/oserrorstring.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:14: `Replace os.ErrorString() casts with calls to
os.NewError().
s/casts/conversions/

At least that's what you always say when I say "cast".  :-)

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:31: // The conversion os.ErrorString(x) looks
like a call
It would be nice at the same time to drop the os.Error in 

var x os.Error = os.ErrorString(y)

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:42: // Other occurences of os.ErrorString are not
fixed;
in occurences, s/r/rr/

[gri1, 2011-06-21 18:55:59]

PTAL

(reverted files w/o manual changes and applied gofix to re-apply the changes):

cd $GOROOT/src/pkg
for f in archive/tar/reader.go big/int.go big/nat.go big/rat.go bytes/buffer.go
compress/gzip/gunzip.go compress/zlib/reader.go crypto/cast5/cast5.go
crypto/dsa/dsa.go crypto/rsa/pkcs1v15.go crypto/rsa/rsa.go crypto/tls/conn.go
crypto/tls/handshake_client.go crypto/tls/handshake_server.go
crypto/tls/key_agreement.go crypto/tls/tls.go crypto/x509/verify_test.go
crypto/x509/x509.go debug/elf/file.go exec/lp_plan9.go exec/lp_unix.go
exec/lp_windows.go fmt/print.go fmt/scan.go fmt/scan_test.go go/build/dir.go
go/parser/interface.go go/types/exportdata.go go/types/gcimporter.go
gob/codec_test.go gob/decoder.go gob/encoder.go gob/gobencdec_test.go
gob/type.go http/client.go http/fs.go http/transport.go http/url.go
image/gif/reader.go mail/message.go net/fd_darwin.go net/iprawsock.go net/net.go
net/udpsock.go netchan/common.go netchan/export.go netchan/import.go
rand/rand_test.go rpc/jsonrpc/all_test.go rpc/server.go rpc/server_test.go
strconv/atoi.go strings/reader.go syslog/syslog_unix.go time/format.go
time/tick.go xml/read.go ; do hg revert $f; gofix $f; done

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.go
File src/cmd/gofix/oserrorstring.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:14: `Replace os.ErrorString() casts with calls to
os.NewError().
On 2011/06/21 16:05:56, rsc wrote:
> s/casts/conversions/
> 
> At least that's what you always say when I say "cast".  :-)

Done.

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:31: // The conversion os.ErrorString(x) looks
like a call
On 2011/06/21 16:05:56, rsc wrote:
> It would be nice at the same time to drop the os.Error in 
> 
> var x os.Error = os.ErrorString(y)

Done.

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:42: // Other occurences of os.ErrorString are not
fixed;
On 2011/06/21 01:21:16, r wrote:
> s/;//

Done.

http://codereview.appspot.com/4607052/diff/4081/src/cmd/gofix/oserrorstring.g...
src/cmd/gofix/oserrorstring.go:42: // Other occurences of os.ErrorString are not
fixed;
On 2011/06/21 16:05:56, rsc wrote:
> in occurences, s/r/rr/

Done.

http://codereview.appspot.com/4607052/diff/4081/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/bufio/bufio.go#newcode33
src/pkg/bufio/bufio.go:33: errInternal          os.Error =
&Error{os.NewError("bufio: internal error")}
On 2011/06/21 01:21:16, r wrote:
> helper function please. what is this, java?

Done.

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/http/request.go#newco...
src/pkg/http/request.go:46: ErrLineTooLong          =
&ProtocolError{os.NewError("header line too long")}
On 2011/06/21 01:21:16, r wrote:
> i'd write a helper function

Done.

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go#n...
src/pkg/websocket/client.go:26: ErrBadScheme            = os.NewError("bad
scheme") // TODO: shouldn't this be a ProtocolError?
On 2011/06/21 01:21:16, r wrote:
> fix TODO before submitting.

Done.

http://codereview.appspot.com/4607052/diff/4081/src/pkg/websocket/client.go#n...
src/pkg/websocket/client.go:27: ErrBadStatus            =
&ProtocolError{os.NewError("bad status")}
On 2011/06/21 01:21:16, r wrote:
> helper function

Done.

[rsc, 2011-06-21 19:03:48]

http://codereview.appspot.com/4607052/diff/7074/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4607052/diff/7074/src/pkg/io/io.go#newcode15
src/pkg/io/io.go:15: os.Error
Instead of this, which I fear will lead to more complex things being stuffed in
here, could you make this

type Error struct {
    String string
}

func (e *Error) String() string { return e.String }

and then the code using it doesn't have to change.
Same in http and bufio and websocket.


Thanks.

[gri1, 2011-06-21 19:12:30]

PTAL

http://codereview.appspot.com/4607052/diff/7074/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4607052/diff/7074/src/pkg/io/io.go#newcode15
src/pkg/io/io.go:15: os.Error
On 2011/06/21 19:03:48, rsc wrote:
> Instead of this, which I fear will lead to more complex things being stuffed
in
> here, could you make this
> 
> type Error struct {
>     String string
> }
> 
> func (e *Error) String() string { return e.String }
> 
> and then the code using it doesn't have to change.
> Same in http and bufio and websocket.
> 
> 
> Thanks.

Done.

[rsc, 2011-06-21 19:14:59]

http://codereview.appspot.com/4607052/diff/20001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/20001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:24: type Error string
For a change like this it is nicest to keep the
delta as minimal as possible.  This should be a
struct with a pointer method, because the old
code was a struct with a pointer method.  
Changing it to string means that code testing
for *bufio.Error, if any does, now must change
to test for bufio.Error.  It also means the
uses all have to change.  If you drop in the five 
lines of struct definition then all the other 
changes in the package can be reverted.

[gri1, 2011-06-21 19:26:34]

PTAL

http://codereview.appspot.com/4607052/diff/20001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/20001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:24: type Error string
On 2011/06/21 19:14:59, rsc wrote:
> For a change like this it is nicest to keep the
> delta as minimal as possible.  This should be a
> struct with a pointer method, because the old
> code was a struct with a pointer method.  
> Changing it to string means that code testing
> for *bufio.Error, if any does, now must change
> to test for bufio.Error.  It also means the
> uses all have to change.  If you drop in the five 
> lines of struct definition then all the other 
> changes in the package can be reverted.
> 

Done.

[rsc, 2011-06-21 19:38:36]

LGTM

This is super picky but since the value are all pointers,
making it a pointer receiver means that

var err os.Error = ...
if e, ok := err.(io.Error); ok {
    
will draw a compiler error instead of silently succeeding
but not actually testing what the programmer meant.

Leaving for Rob.

http://codereview.appspot.com/4607052/diff/20003/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:28: func (err Error) String() string { return
err.ErrorString }
err *Error

http://codereview.appspot.com/4607052/diff/20003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/http/request.go#newc...
src/pkg/http/request.go:45: func (err ProtocolError) String() string { return
err.ErrorString }
err *ProtocolError

http://codereview.appspot.com/4607052/diff/20003/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/io/io.go#newcode18
src/pkg/io/io.go:18: func (err Error) String() string { return err.ErrorString }
err *Error

http://codereview.appspot.com/4607052/diff/20003/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:25: func (err ProtocolError) String() string {
return string(err.ErrorString) }
err *ProtocolError

[gri1, 2011-06-21 21:01:53]

PTAL.

Makes sense, I should have used ptr receivers in the first place. Thanks for the
thorough review.

http://codereview.appspot.com/4607052/diff/20003/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:28: func (err Error) String() string { return
err.ErrorString }
On 2011/06/21 19:38:36, rsc wrote:
> err *Error

Done.

http://codereview.appspot.com/4607052/diff/20003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/http/request.go#newc...
src/pkg/http/request.go:45: func (err ProtocolError) String() string { return
err.ErrorString }
On 2011/06/21 19:38:36, rsc wrote:
> err *ProtocolError

Done.

http://codereview.appspot.com/4607052/diff/20003/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/io/io.go#newcode18
src/pkg/io/io.go:18: func (err Error) String() string { return err.ErrorString }
On 2011/06/21 19:38:36, rsc wrote:
> err *Error

Done.

http://codereview.appspot.com/4607052/diff/20003/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4607052/diff/20003/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:25: func (err ProtocolError) String() string {
return string(err.ErrorString) }
On 2011/06/21 19:38:36, rsc wrote:
> err *ProtocolError

Done.

[r, 2011-06-22 00:27:24]

LGTM

[gri, 2011-06-22 17:53:00]

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

os.Error API: don't export os.ErrorString, use os.NewError consistently

This is a core API change.

1) gofix misc src
2) Manual adjustments to the following files under src/pkg:
   gob/decode.go
   rpc/client.go
   os/error.go
   io/io.go
   bufio/bufio.go
   http/request.go
   websocket/client.go
as well as:
   src/cmd/gofix/testdata/*.go.in (reverted)
   test/fixedbugs/bug243.go
3) Implemented gofix patch (oserrorstring.go) and test case
(oserrorstring_test.go)

Compiles and runs all tests.

R=r, rsc, gri
CC=golang-dev
http://codereview.appspot.com/4607052