code review 223076: gofmt: experiment: align values in map composites where...

gofmt: experiment: align values in map composites where possible
- gofmt -w src misc
- looking for feedback

[gri, 2010-02-27 02:53:59]

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

I'd like you to review this change.

[gri, 2010-02-27 03:01:04]

The only manual change is in src/pkg/go/printer/nodes.go . All other files
were gofmt'ed. Some maps look much better, for some it doesn't matter.

One could align the ":" (similarly to the "="'s in constant declarations)
but that's actually non-trivial unless one accepts a leading blank before
the ":". So "foo : bar" instead of "foo: bar".

Anyway, I'd probably submit this if it were just me, but I don't feel
strongly about it if you don't like it.
- Robert


On Fri, Feb 26, 2010 at 6:53 PM, <gri@golang.org> wrote:

> Reviewers: rsc, r,
>
> Message:
> Hello rsc, r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> gofmt: experiment: align values in map composites where possible
> - gofmt -w src misc
> - looking for feedback
>
> Please review this at http://codereview.appspot.com/223076/show
>
> Affected files:
>  M src/cmd/cgo/ast.go
>  M src/cmd/cgo/gcc.go
>  M src/cmd/cgo/main.go
>  M src/cmd/godoc/godoc.go
>  M src/cmd/godoc/index.go
>  M src/pkg/archive/tar/reader_test.go
>  M src/pkg/archive/tar/writer_test.go
>  M src/pkg/asn1/marshal.go
>  M src/pkg/big/int_test.go
>  M src/pkg/compress/flate/huffman_bit_writer.go
>  M src/pkg/compress/flate/huffman_code.go
>  M src/pkg/crypto/tls/handshake_client.go
>  M src/pkg/crypto/x509/x509.go
>  M src/pkg/crypto/x509/x509_test.go
>  M src/pkg/debug/dwarf/const.go
>  M src/pkg/debug/dwarf/entry.go
>  M src/pkg/debug/dwarf/open.go
>  M src/pkg/debug/dwarf/type_test.go
>  M src/pkg/debug/elf/file.go
>  M src/pkg/debug/proc/proc_linux.go
>  M src/pkg/encoding/pem/pem.go
>  M src/pkg/encoding/pem/pem_test.go
>  M src/pkg/exp/eval/compiler.go
>  M src/pkg/exp/eval/expr.go
>  M src/pkg/exp/eval/gen.go
>  M src/pkg/exp/eval/scope.go
>  M src/pkg/exp/eval/stmt.go
>  M src/pkg/exp/eval/world.go
>  M src/pkg/exp/nacl/srpc/client.go
>  M src/pkg/exp/nacl/srpc/msg.go
>  M src/pkg/exp/ogle/process.go
>  M src/pkg/exp/ogle/rruntime.go
>  M src/pkg/exp/spacewar/spacewar.go
>  M src/pkg/go/printer/nodes.go
>  M src/pkg/go/printer/testdata/declarations.golden
>  M src/pkg/go/printer/testdata/linebreaks.golden
>  M src/pkg/go/token/token.go
>  M src/pkg/gob/codec_test.go
>  M src/pkg/gob/decode.go
>  M src/pkg/gob/encode.go
>  M src/pkg/http/lex_test.go
>  M src/pkg/http/readrequest_test.go
>  M src/pkg/http/request.go
>  M src/pkg/http/request_test.go
>  M src/pkg/http/requestwrite_test.go
>  M src/pkg/http/response.go
>  M src/pkg/http/response_test.go
>  M src/pkg/http/responsewrite_test.go
>  M src/pkg/http/status.go
>  M src/pkg/mime/mime_test.go
>  M src/pkg/mime/type.go
>  M src/pkg/net/dnsmsg.go
>  M src/pkg/net/fd.go
>  M src/pkg/netchan/export.go
>  M src/pkg/reflect/value.go
>  M src/pkg/scanner/scanner.go
>  M src/pkg/syscall/zerrors_darwin_386.go
>  M src/pkg/syscall/zerrors_darwin_amd64.go
>  M src/pkg/syscall/zerrors_freebsd_386.go
>  M src/pkg/syscall/zerrors_freebsd_amd64.go
>  M src/pkg/syscall/zerrors_linux_386.go
>  M src/pkg/syscall/zerrors_linux_amd64.go
>  M src/pkg/syscall/zerrors_linux_arm.go
>  M src/pkg/syscall/zerrors_nacl_386.go
>  M src/pkg/template/template.go
>  M src/pkg/template/template_test.go
>  M src/pkg/unicode/tables.go
>  M src/pkg/xgb/xgb.go
>  M src/pkg/xgb/xproto.go
>  M src/pkg/xml/read_test.go
>  M src/pkg/xml/xml.go
>  M src/pkg/xml/xml_test.go
>
>
>

[r2, 2010-02-27 06:03:03]

can you point me at a few you think look particularly different?

-rob

On 27/02/2010, at 1:53 PM, gri@golang.org wrote:

> Reviewers: rsc, r,
> 
> Message:
> Hello rsc, r (cc: golang-dev@googlegroups.com),
> 
> I'd like you to review this change.
> 
> 
> Description:
> gofmt: experiment: align values in map composites where possible
> - gofmt -w src misc
> - looking for feedback
> 
> Please review this at http://codereview.appspot.com/223076/show
> 
> Affected files:
>  M src/cmd/cgo/ast.go
>  M src/cmd/cgo/gcc.go
>  M src/cmd/cgo/main.go
>  M src/cmd/godoc/godoc.go
>  M src/cmd/godoc/index.go
>  M src/pkg/archive/tar/reader_test.go
>  M src/pkg/archive/tar/writer_test.go
>  M src/pkg/asn1/marshal.go
>  M src/pkg/big/int_test.go
>  M src/pkg/compress/flate/huffman_bit_writer.go
>  M src/pkg/compress/flate/huffman_code.go
>  M src/pkg/crypto/tls/handshake_client.go
>  M src/pkg/crypto/x509/x509.go
>  M src/pkg/crypto/x509/x509_test.go
>  M src/pkg/debug/dwarf/const.go
>  M src/pkg/debug/dwarf/entry.go
>  M src/pkg/debug/dwarf/open.go
>  M src/pkg/debug/dwarf/type_test.go
>  M src/pkg/debug/elf/file.go
>  M src/pkg/debug/proc/proc_linux.go
>  M src/pkg/encoding/pem/pem.go
>  M src/pkg/encoding/pem/pem_test.go
>  M src/pkg/exp/eval/compiler.go
>  M src/pkg/exp/eval/expr.go
>  M src/pkg/exp/eval/gen.go
>  M src/pkg/exp/eval/scope.go
>  M src/pkg/exp/eval/stmt.go
>  M src/pkg/exp/eval/world.go
>  M src/pkg/exp/nacl/srpc/client.go
>  M src/pkg/exp/nacl/srpc/msg.go
>  M src/pkg/exp/ogle/process.go
>  M src/pkg/exp/ogle/rruntime.go
>  M src/pkg/exp/spacewar/spacewar.go
>  M src/pkg/go/printer/nodes.go
>  M src/pkg/go/printer/testdata/declarations.golden
>  M src/pkg/go/printer/testdata/linebreaks.golden
>  M src/pkg/go/token/token.go
>  M src/pkg/gob/codec_test.go
>  M src/pkg/gob/decode.go
>  M src/pkg/gob/encode.go
>  M src/pkg/http/lex_test.go
>  M src/pkg/http/readrequest_test.go
>  M src/pkg/http/request.go
>  M src/pkg/http/request_test.go
>  M src/pkg/http/requestwrite_test.go
>  M src/pkg/http/response.go
>  M src/pkg/http/response_test.go
>  M src/pkg/http/responsewrite_test.go
>  M src/pkg/http/status.go
>  M src/pkg/mime/mime_test.go
>  M src/pkg/mime/type.go
>  M src/pkg/net/dnsmsg.go
>  M src/pkg/net/fd.go
>  M src/pkg/netchan/export.go
>  M src/pkg/reflect/value.go
>  M src/pkg/scanner/scanner.go
>  M src/pkg/syscall/zerrors_darwin_386.go
>  M src/pkg/syscall/zerrors_darwin_amd64.go
>  M src/pkg/syscall/zerrors_freebsd_386.go
>  M src/pkg/syscall/zerrors_freebsd_amd64.go
>  M src/pkg/syscall/zerrors_linux_386.go
>  M src/pkg/syscall/zerrors_linux_amd64.go
>  M src/pkg/syscall/zerrors_linux_arm.go
>  M src/pkg/syscall/zerrors_nacl_386.go
>  M src/pkg/template/template.go
>  M src/pkg/template/template_test.go
>  M src/pkg/unicode/tables.go
>  M src/pkg/xgb/xgb.go
>  M src/pkg/xgb/xproto.go
>  M src/pkg/xml/read_test.go
>  M src/pkg/xml/xml.go
>  M src/pkg/xml/xml_test.go
> 
> 

[gri, 2010-02-27 20:04:57]

For instance:

These seem nicer (easier to read):

http://codereview.appspot.com/223076/diff/219/1139
http://codereview.appspot.com/223076/diff/219/1087
http://codereview.appspot.com/223076/diff/219/1109

Here it doesn't really matter:

http://codereview.appspot.com/223076/diff/219/1093
http://codereview.appspot.com/223076/diff/219/1095
http://codereview.appspot.com/223076/diff/219/1129

There is one file which may be worse (it wasn't great before either):

http://codereview.appspot.com/223076/diff/219/1094

- Robert

On Fri, Feb 26, 2010 at 10:02 PM, Rob 'Commander' Pike <r@google.com> wrote:

> can you point me at a few you think look particularly different?
>
> -rob
>
> On 27/02/2010, at 1:53 PM, gri@golang.org wrote:
>
> > Reviewers: rsc, r,
> >
> > Message:
> > Hello rsc, r (cc: golang-dev@googlegroups.com),
> >
> > I'd like you to review this change.
> >
> >
> > Description:
> > gofmt: experiment: align values in map composites where possible
> > - gofmt -w src misc
> > - looking for feedback
> >
> > Please review this at http://codereview.appspot.com/223076/show
> >
> > Affected files:
> >  M src/cmd/cgo/ast.go
> >  M src/cmd/cgo/gcc.go
> >  M src/cmd/cgo/main.go
> >  M src/cmd/godoc/godoc.go
> >  M src/cmd/godoc/index.go
> >  M src/pkg/archive/tar/reader_test.go
> >  M src/pkg/archive/tar/writer_test.go
> >  M src/pkg/asn1/marshal.go
> >  M src/pkg/big/int_test.go
> >  M src/pkg/compress/flate/huffman_bit_writer.go
> >  M src/pkg/compress/flate/huffman_code.go
> >  M src/pkg/crypto/tls/handshake_client.go
> >  M src/pkg/crypto/x509/x509.go
> >  M src/pkg/crypto/x509/x509_test.go
> >  M src/pkg/debug/dwarf/const.go
> >  M src/pkg/debug/dwarf/entry.go
> >  M src/pkg/debug/dwarf/open.go
> >  M src/pkg/debug/dwarf/type_test.go
> >  M src/pkg/debug/elf/file.go
> >  M src/pkg/debug/proc/proc_linux.go
> >  M src/pkg/encoding/pem/pem.go
> >  M src/pkg/encoding/pem/pem_test.go
> >  M src/pkg/exp/eval/compiler.go
> >  M src/pkg/exp/eval/expr.go
> >  M src/pkg/exp/eval/gen.go
> >  M src/pkg/exp/eval/scope.go
> >  M src/pkg/exp/eval/stmt.go
> >  M src/pkg/exp/eval/world.go
> >  M src/pkg/exp/nacl/srpc/client.go
> >  M src/pkg/exp/nacl/srpc/msg.go
> >  M src/pkg/exp/ogle/process.go
> >  M src/pkg/exp/ogle/rruntime.go
> >  M src/pkg/exp/spacewar/spacewar.go
> >  M src/pkg/go/printer/nodes.go
> >  M src/pkg/go/printer/testdata/declarations.golden
> >  M src/pkg/go/printer/testdata/linebreaks.golden
> >  M src/pkg/go/token/token.go
> >  M src/pkg/gob/codec_test.go
> >  M src/pkg/gob/decode.go
> >  M src/pkg/gob/encode.go
> >  M src/pkg/http/lex_test.go
> >  M src/pkg/http/readrequest_test.go
> >  M src/pkg/http/request.go
> >  M src/pkg/http/request_test.go
> >  M src/pkg/http/requestwrite_test.go
> >  M src/pkg/http/response.go
> >  M src/pkg/http/response_test.go
> >  M src/pkg/http/responsewrite_test.go
> >  M src/pkg/http/status.go
> >  M src/pkg/mime/mime_test.go
> >  M src/pkg/mime/type.go
> >  M src/pkg/net/dnsmsg.go
> >  M src/pkg/net/fd.go
> >  M src/pkg/netchan/export.go
> >  M src/pkg/reflect/value.go
> >  M src/pkg/scanner/scanner.go
> >  M src/pkg/syscall/zerrors_darwin_386.go
> >  M src/pkg/syscall/zerrors_darwin_amd64.go
> >  M src/pkg/syscall/zerrors_freebsd_386.go
> >  M src/pkg/syscall/zerrors_freebsd_amd64.go
> >  M src/pkg/syscall/zerrors_linux_386.go
> >  M src/pkg/syscall/zerrors_linux_amd64.go
> >  M src/pkg/syscall/zerrors_linux_arm.go
> >  M src/pkg/syscall/zerrors_nacl_386.go
> >  M src/pkg/template/template.go
> >  M src/pkg/template/template_test.go
> >  M src/pkg/unicode/tables.go
> >  M src/pkg/xgb/xgb.go
> >  M src/pkg/xgb/xproto.go
> >  M src/pkg/xml/read_test.go
> >  M src/pkg/xml/xml.go
> >  M src/pkg/xml/xml_test.go
> >
> >
>
>

[rsc, 2010-02-27 23:33:07]

I don't mind either way.
The giant tables definitely look nicer.
(Though I'd skip lining up the colons.)

I haven't looked at the code changes at all.
What is the heuristic that keeps this change from
reformatting the definition of var decode in
src/pkg/encoding/git85/git.go?

Russ

[rsc, 2010-02-27 23:53:59]

Also, it seems like the pem_test.go formatting
(all the extra tabs) is worth trying to fix.  It's easy
to believe that people will write things like

T{a:1, b:2, c:3, d:4,
    e:5, f:6, g:7}

[gri, 2010-03-02 00:36:44]

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

Please take another look.

[gri, 2010-03-02 00:37:33]

On Sat, Feb 27, 2010 at 3:33 PM, Russ Cox <rsc@golang.org> wrote:

> I don't mind either way.
> The giant tables definitely look nicer.
> (Though I'd skip lining up the colons.)
>
> I haven't looked at the code changes at all.
> What is the heuristic that keeps this change from
> reformatting the definition of var decode in
> src/pkg/encoding/git85/git.go?
>

This is a somewhat special case. At the moment I am only looking at simple
situations where each expression is on a new line. One could handle this
case as well, but I think it would require it's own heuristic. Another CL
perhaps?

>
> Russ
>
>

[gri, 2010-03-02 00:38:25]

I believe this is fixed now; i.e., there are no cases anymore that are worse
than before.
- Robert

On Sat, Feb 27, 2010 at 3:53 PM, Russ Cox <rsc@golang.org> wrote:

> Also, it seems like the pem_test.go formatting
> (all the extra tabs) is worth trying to fix.  It's easy
> to believe that people will write things like
>
> T{a:1, b:2, c:3, d:4,
>    e:5, f:6, g:7}
>

[r, 2010-03-02 00:58:13]

be nice to fix the ugly case. rolling this out will reformat a ton of stuff so
it would be nice to do it once rather than incrementally.

but i mostly like it a lot.

http://codereview.appspot.com/223076/diff/219/1094
File src/pkg/encoding/pem/pem_test.go (right):

http://codereview.appspot.com/223076/diff/219/1094#newcode176
src/pkg/encoding/pem/pem_test.go:176: Bytes: []uint8{0x30, 0x82, 0x3, 0xe9,
0x30, 0x82, 0x3, 0x52, 0x2, 0x1,
i don't understand this alignment.  why isn't the Headers under the Type?  in
other words, why all this indentation to unalign it?

[gri, 2010-03-02 01:17:01]

On Mon, Mar 1, 2010 at 4:58 PM, <r@golang.org> wrote:

> be nice to fix the ugly case. rolling this out will reformat a ton of
> stuff so it would be nice to do it once rather than incrementally.
>

It's fixed, I believe you are looking at an old version. This file is not
changed anymore.

>
> but i mostly like it a lot.
>
>
> http://codereview.appspot.com/223076/diff/219/1094
> File src/pkg/encoding/pem/pem_test.go (right):
>
> http://codereview.appspot.com/223076/diff/219/1094#newcode176
> src/pkg/encoding/pem/pem_test.go:176: Bytes: []uint8{0x30, 0x82, 0x3,
> 0xe9, 0x30, 0x82, 0x3, 0x52, 0x2, 0x1,
> i don't understand this alignment.  why isn't the Headers under the
> Type?  in other words, why all this indentation to unalign it?
>
>
> http://codereview.appspot.com/223076/show
>

See above.
- Robert

[r2, 2010-03-02 01:20:25]

LGTM then if rsc approves

On Mar 1, 2010, at 5:16 PM, Robert Griesemer wrote:

> On Mon, Mar 1, 2010 at 4:58 PM, <r@golang.org> wrote:
> be nice to fix the ugly case. rolling this out will reformat a ton of
> stuff so it would be nice to do it once rather than incrementally.
> 
> It's fixed, I believe you are looking at an old version. This file is not
changed anymore. 
> 
> but i mostly like it a lot.
> 
> 
> http://codereview.appspot.com/223076/diff/219/1094
> File src/pkg/encoding/pem/pem_test.go (right):
> 
> http://codereview.appspot.com/223076/diff/219/1094#newcode176
> src/pkg/encoding/pem/pem_test.go:176: Bytes: []uint8{0x30, 0x82, 0x3,
> 0xe9, 0x30, 0x82, 0x3, 0x52, 0x2, 0x1,
> i don't understand this alignment.  why isn't the Headers under the
> Type?  in other words, why all this indentation to unalign it?
> 
> 
> http://codereview.appspot.com/223076/show
> 
> See above.
> - Robert

[rsc, 2010-03-02 21:38:36]

LGTM

[gri, 2010-03-02 21:46:59]

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

gofmt: experiment: align values in map composites where possible
- gofmt -w src misc
- looking for feedback

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