code review 5753055: fmt, log: stop using unicode

fmt, log: stop using unicode

$ go list -f '{{.ImportPath}} {{.Deps}}' fmt log
fmt [errors io math os reflect runtime strconv sync sync/atomic syscall time unicode/utf8 unsafe]
log [errors fmt io math os reflect runtime strconv sync sync/atomic syscall time unicode/utf8 unsafe]

[bradfitz, 2012-03-06 16:08:07]

LGTM

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/format.go
File src/pkg/fmt/format.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/format.go#newcode291
src/pkg/fmt/format.go:291: var b []byte
seems like the cap could be set pretty accurately here?

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/print.go#newcode89
src/pkg/fmt/print.go:89: func (bp *buffer) WriteRune(r rune) error {
use bp as receiver in methods above?
or bs as byte slice below?

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode278
src/pkg/fmt/scan.go:278: var space = []uint16{
// space lists pairs of inclusive ranges of space characters

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode422
src/pkg/fmt/scan.go:422: s.buf = s.buf[:0]
can't you just add a Reset method to *buffer?

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode710
src/pkg/fmt/scan.go:710: s.buf = s.buf[:0]
Reset?

[rog, 2012-03-06 16:46:48]

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode278
src/pkg/fmt/scan.go:278: var space = []uint16{
this can be a little bit more efficient if defined as [][2]uint16, because you
can avoid all the bounds checks in loop below.

[r, 2012-03-06 20:40:34]

holding off LGTM until strconv has landed

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/format.go
File src/pkg/fmt/format.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/format.go#newcode37
src/pkg/fmt/format.go:37: // It prints into a buffer that must be set up
externally.
s/externally/separately/

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode422
src/pkg/fmt/scan.go:422: s.buf = s.buf[:0]
On 2012/03/06 16:08:07, bradfitz wrote:
> can't you just add a Reset method to *buffer?

to save a trivial assignment?

http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode481
src/pkg/fmt/scan.go:481: func indexRune(s string, r rune) int {
should be indexByte. it's used for tiny ASCII strings.

[bradfitz, 2012-03-06 20:44:29]

On Wed, Mar 7, 2012 at 7:40 AM, <r@golang.org> wrote:

> holding off LGTM until strconv has landed
>
>
>
>
http://codereview.appspot.com/**5753055/diff/2001/src/pkg/fmt/**format.go<htt...
> File src/pkg/fmt/format.go (right):
>
> http://codereview.appspot.com/**5753055/diff/2001/src/pkg/fmt/**
>
format.go#newcode37<http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/format.go#newcode37>
> src/pkg/fmt/format.go:37: // It prints into a buffer that must be set up
> externally.
> s/externally/separately/
>
>
>
http://codereview.appspot.com/**5753055/diff/2001/src/pkg/fmt/**scan.go<http:...
> File src/pkg/fmt/scan.go (right):
>
> http://codereview.appspot.com/**5753055/diff/2001/src/pkg/fmt/**
>
scan.go#newcode422<http://codereview.appspot.com/5753055/diff/2001/src/pkg/fmt/scan.go#newcode422>
> src/pkg/fmt/scan.go:422: s.buf = s.buf[:0]
> On 2012/03/06 16:08:07, bradfitz wrote:
>
>> can't you just add a Reset method to *buffer?
>>
>
> to save a trivial assignment?
>

*shrug*  There's a number of them. Keeps diff smaller and more readable
overall IMHO. (showing intent, not implementation)

[r2, 2012-03-06 20:47:14]

also this should include edits to the dep test

[rsc, 2012-03-07 03:41:03]

Hello bradfitz@golang.org, rogpeppe@gmail.com, r@golang.org, r@google.com (cc:
golang-dev@googlegroups.com),

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

[r, 2012-03-07 03:53:04]

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/format.go
File src/pkg/fmt/format.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/format.go#newcode37
src/pkg/fmt/format.go:37: // It prints into a buffer that must be set up
externally.
did you even see my earlier comments? s/externally/separately/

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/print.go#newcode72
src/pkg/fmt/print.go:72: type buffer []byte
maybe // Use buffer instead of bytes.Buffer to avoid large dependency.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go#newcode42
src/pkg/fmt/scan.go:42: // !isSpace(c) is used; that is, the token will hold
non-space
you're describing a public interface in terms of a private function.
i think you can just revert this line. if you're worried about the implication
of dependency, say

If f is nil, tokens will hold runs of non-space characters, as defined by
Unicode.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go#newcode278
src/pkg/fmt/scan.go:278: var space = []uint16{
i'd prefer to see these grouped as ranges.
at the very least, put in a comment and observe that this needs
to be kept in sync with the unicode White_Space range table.
plus there should be a test to diff the tables or scan the range.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go#newcode481
src/pkg/fmt/scan.go:481: func indexRune(s string, r rune) int {
as i said before, this (c|sh)ould be indexByte.

[rsc1, 2012-03-07 04:12:24]

PTAL

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/format.go
File src/pkg/fmt/format.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/format.go#newcode37
src/pkg/fmt/format.go:37: // It prints into a buffer that must be set up
externally.
On 2012/03/07 03:53:05, r wrote:
> did you even see my earlier comments? s/externally/separately/

Very sorry.  I forgot about that mail.
Done.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/print.go#newcode72
src/pkg/fmt/print.go:72: type buffer []byte
On 2012/03/07 03:53:05, r wrote:
> maybe // Use buffer instead of bytes.Buffer to avoid large dependency.

Done.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go#newcode42
src/pkg/fmt/scan.go:42: // !isSpace(c) is used; that is, the token will hold
non-space
On 2012/03/07 03:53:05, r wrote:
> you're describing a public interface in terms of a private function.
> i think you can just revert this line. if you're worried about the implication
> of dependency, say
> 
> If f is nil, tokens will hold runs of non-space characters, as defined by
> Unicode.

Done.

http://codereview.appspot.com/5753055/diff/4008/src/pkg/fmt/scan.go#newcode481
src/pkg/fmt/scan.go:481: func indexRune(s string, r rune) int {
On 2012/03/07 03:53:05, r wrote:
> as i said before, this (c|sh)ould be indexByte.

Sorry for not replying earlier.  My reasoning was that
r is the result of s.getRune(), which can be an arbitrary
rune.  I could check at the call sites for r >= RuneSelf
and then call IndexByte, or I could check rune(s[i]) == r?

[r, 2012-03-07 04:17:28]

LGTM

[rsc, 2012-03-07 04:27:17]

*** Submitted as 61a5159136af ***

fmt, log: stop using unicode

$ go list -f '{{.ImportPath}} {{.Deps}}' fmt log
fmt [errors io math os reflect runtime strconv sync sync/atomic syscall time
unicode/utf8 unsafe]
log [errors fmt io math os reflect runtime strconv sync sync/atomic syscall time
unicode/utf8 unsafe]

R=bradfitz, rogpeppe, r, r, rsc
CC=golang-dev
http://codereview.appspot.com/5753055