code review 7205047: fmt: improve go syntax handling of byte-derived arrays ...

fmt: improve go syntax handling of byte-derived arrays and slices

Fixes issue 4685.

[kortschak, 2013-01-24 01:03:03]

Hello golang-dev@googlegroups.com,

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

[kevlar, 2013-01-25 09:08:35]

FYI

https://codereview.appspot.com/7205047/diff/3/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/3/src/pkg/fmt/print.go#newcode942
src/pkg/fmt/print.go:942: if f.Type().Elem() == reflect.TypeOf(byte(0)) {
I think you might want to keep the == reflect.Uint8 but then also ensure that
.PkgPath() == "" (aka it's a predeclared type):
http://play.golang.org/p/5DTBpto305

Both of these are direct comparisons, instead of computing TypeOf each time and
requiring an interface comparison.

[kortschak, 2013-01-25 10:56:33]

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

Please take another look.

[kortschak, 2013-01-25 11:07:36]

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go#newcode942
src/pkg/fmt/print.go:942: if f.Type().Elem().Kind() == reflect.Uint8 &&
f.Type().Elem().PkgPath() == "" {
This is two calls to .Type().Elem() rather than storing in an intermediate
variable since this approach was very slightly faster in a micro-benchmark.
Conciseness could easily trump this.

[adg, 2013-01-29 07:33:15]

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go#newcode942
src/pkg/fmt/print.go:942: if f.Type().Elem().Kind() == reflect.Uint8 &&
f.Type().Elem().PkgPath() == "" {
On 2013/01/25 11:07:37, kortschak wrote:
> Conciseness could easily trump this.

IMO it does. We should make the compiler better, not write verbose code.

[remyoudompheng, 2013-01-29 07:49:55]

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/7001/src/pkg/fmt/print.go#newcode950
src/pkg/fmt/print.go:950: bytes := make([]byte, f.Len())
I wonder why this is not

if f.Kind() == reflect.Array {
    f = f.Slice(0, f.Len())
}
bytes := f.Bytes()

if using this (which is also faster), the large comment should be removed.

[kortschak, 2013-01-29 10:53:13]

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

Please take another look.

[rsc, 2013-01-30 17:13:26]

not lgtm

This breaks %q on []myByte. fmtBytes still needs to be called. It should
just be passed an additional argument specifying the original type for use
in %#v.

[kortschak, 2013-01-30 19:36:05]

On 2013/01/30 17:13:26, rsc wrote:
> not lgtm
> 
> This breaks %q on []myByte. fmtBytes still needs to be called. It should
> just be passed an additional argument specifying the original type for use
> in %#v.

What is the expected behaviour for %q on []myByte? There is no test case for
this in fmt. My reasoning was that since a []myByte behaves everywhere else as
completely different to a []byte except for size and sign, it would probably
behave differently here too.

[rsc, 2013-01-30 19:49:20]

I believe %q on []myByte should behave the same way it currently does, that
the goal for the CL should be to fix the type shown in %#v and leave
everything else unchanged.

Russ

[kortschak, 2013-01-30 19:56:37]

OK. That's a pity. I'll rework this.

[kortschak, 2013-01-30 20:05:33]

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

Please take another look.

[kortschak, 2013-01-30 20:07:39]

https://codereview.appspot.com/7205047/diff/19001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/19001/src/pkg/fmt/print.go#newcod...
src/pkg/fmt/print.go:942: if e := f.Type().Elem(); e.Kind() == reflect.Uint8 &&
(e.PkgPath() == "" || verb == 'q') {
This is getting verbose, but I thought it less invasive that passing type
information to fmtBytes.

[kortschak, 2013-01-30 20:28:55]

This breaks still %s, %xand %X.

I will pass type to fmtBytes.

[rsc, 2013-01-30 20:32:33]

Please pass a new reflect.Type argument to fmtBytes. That should be enough.

https://codereview.appspot.com/7205047/diff/19001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/19001/src/pkg/fmt/print.go#newcod...
src/pkg/fmt/print.go:942: if e := f.Type().Elem(); e.Kind() == reflect.Uint8 &&
(e.PkgPath() == "" || verb == 'q') {
Okay but now it's broken for %s.

[kortschak, 2013-01-30 22:00:12]

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

Please take another look.

[kortschak, 2013-01-30 22:01:16]

https://codereview.appspot.com/7205047/diff/18002/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

https://codereview.appspot.com/7205047/diff/18002/src/pkg/fmt/print.go#newcod...
src/pkg/fmt/print.go:556: p.buf.WriteByte('{')
Sorry. Too early.

[kortschak, 2013-01-30 22:05:41]

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

Please take another look.

[rsc, 2013-01-31 01:51:14]

LGTM

[rsc, 2013-01-31 01:53:56]

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

fmt: improve go syntax handling of byte-derived arrays and slices

Fixes issue 4685.

R=golang-dev, adg, remyoudompheng, rsc
CC=golang-dev
https://codereview.appspot.com/7205047

Committer: Russ Cox <rsc@golang.org>