code review 5449071: various: shorten composite literal field values (issue 5449071)

[r...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

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

Description:
various: shorten composite literal field values

Please review this at http://codereview.appspot.com/5449071/

Affected files:
M src/pkg/archive/zip/writer.go
M src/pkg/encoding/xml/xml.go
M src/pkg/exp/norm/readwriter.go
M src/pkg/exp/sql/sql.go
M src/pkg/exp/ssh/transport.go
M src/pkg/go/build/build.go
M src/pkg/html/parse.go
M src/pkg/html/template/escape.go
M src/pkg/math/big/rat.go
M src/pkg/old/netchan/export.go
M src/pkg/testing/benchmark.go
M src/pkg/text/template/exec.go

Index: src/pkg/archive/zip/writer.go
===================================================================
--- a/src/pkg/archive/zip/writer.go
+++ b/src/pkg/archive/zip/writer.go
@@ -32,7 +32,7 @@

// NewWriter returns a new Writer writing a zip file to w.
func NewWriter(w io.Writer) *Writer {
- return &Writer{countWriter: &countWriter{w: bufio.NewWriter(w)}}
+ return &Writer{countWriter: {w: bufio.NewWriter(w)}}
}

// Close finishes writing the zip file by writing the central directory.
@@ -120,7 +120,7 @@

fw := &fileWriter{
zipw: w,
- compCount: &countWriter{w: w},
+ compCount: {w: w},
crc32: crc32.NewIEEE(),
}
switch fh.Method {
Index: src/pkg/encoding/xml/xml.go
===================================================================
--- a/src/pkg/encoding/xml/xml.go
+++ b/src/pkg/encoding/xml/xml.go
@@ -1036,7 +1036,7 @@
// and second corresponds to NameChar.

var first = &unicode.RangeTable{
- R16: []unicode.Range16{
+ R16: {
{0x003A, 0x003A, 1},
{0x0041, 0x005A, 1},
{0x005F, 0x005F, 1},
@@ -1231,7 +1231,7 @@
}

var second = &unicode.RangeTable{
- R16: []unicode.Range16{
+ R16: {
{0x002D, 0x002E, 1},
{0x0030, 0x0039, 1},
{0x00B7, 0x00B7, 1},
Index: src/pkg/exp/norm/readwriter.go
===================================================================
--- a/src/pkg/exp/norm/readwriter.go
+++ b/src/pkg/exp/norm/readwriter.go
@@ -64,7 +64,7 @@
// an internal buffer to maintain state across Write calls.
// Calling its Close method writes any buffered data to w.
func (f Form) Writer(w io.Writer) io.WriteCloser {
- wr := &normWriter{rb: reorderBuffer{}, w: w}
+ wr := &normWriter{rb: {}, w: w}
wr.rb.init(f, nil)
return wr
}
@@ -120,7 +120,7 @@
func (f Form) Reader(r io.Reader) io.Reader {
const chunk = 4000
buf := make([]byte, chunk)
- rr := &normReader{rb: reorderBuffer{}, r: r, inbuf: buf}
+ rr := &normReader{rb: {}, r: r, inbuf: buf}
rr.rb.init(f, buf)
return rr
}
Index: src/pkg/exp/sql/sql.go
===================================================================
--- a/src/pkg/exp/sql/sql.go
+++ b/src/pkg/exp/sql/sql.go
@@ -195,7 +195,7 @@
stmt := &Stmt{
db: db,
query: query,
- css: []connStmt{{ci, si}},
+ css: {{ci, si}},
}
return stmt, nil
}
Index: src/pkg/exp/ssh/transport.go
===================================================================
--- a/src/pkg/exp/ssh/transport.go
+++ b/src/pkg/exp/ssh/transport.go
@@ -221,17 +221,17 @@

func newTransport(conn net.Conn, rand io.Reader) *transport {
return &transport{
- reader: reader{
+ reader: {
Reader: bufio.NewReader(conn),
- common: common{
+ common: {
cipher: noneCipher{},
},
},
- writer: writer{
+ writer: {
Writer: bufio.NewWriter(conn),
rand: rand,
Mutex: new(sync.Mutex),
- common: common{
+ common: {
cipher: noneCipher{},
},
},
Index: src/pkg/go/build/build.go
===================================================================
--- a/src/pkg/go/build/build.go
+++ b/src/pkg/go/build/build.go
@@ -288,8 +288,8 @@

func (b *build) mkdir(name string) {
b.add(Cmd{
- Args: []string{"mkdir", "-p", name},
- Output: []string{name},
+ Args: {"mkdir", "-p", name},
+ Output: {name},
})
}

@@ -300,16 +300,16 @@
b.add(Cmd{
Args: args,
Input: gofiles,
- Output: []string{ofile},
+ Output: {ofile},
})
}

func (b *build) asm(ofile string, sfile string) {
asm := b.arch + "a"
b.add(Cmd{
- Args: []string{asm, "-o", ofile, sfile},
- Input: []string{sfile},
- Output: []string{ofile},
+ Args: {asm, "-o", ofile, sfile},
+ Input: {sfile},
+ Output: {ofile},
})
}

@@ -320,7 +320,7 @@
b.add(Cmd{
Args: args,
Input: ofiles,
- Output: []string{targ},
+ Output: {targ},
})
}

@@ -328,7 +328,7 @@
b.add(Cmd{
Args: append([]string{"gopack", "grc", targ}, ofiles...),
Input: ofiles,
- Output: []string{targ},
+ Output: {targ},
})
}

@@ -340,15 +340,15 @@
b.add(Cmd{
Args: append(args, cfiles...),
Input: cfiles,
- Output: []string{ofile},
+ Output: {ofile},
})
}

func (b *build) gccCompile(ofile, cfile string) {
b.add(Cmd{
Args: b.gccArgs("-o", ofile, "-c", cfile),
- Input: []string{cfile},
- Output: []string{ofile},
+ Input: {cfile},
+ Output: {ofile},
})
}

@@ -356,7 +356,7 @@
b.add(Cmd{
Args: append(b.gccArgs("-o", ofile), ofiles...),
Input: ofiles,
- Output: []string{ofile},
+ Output: {ofile},
})
}

@@ -430,10 +430,10 @@
// cgo -dynimport
importC := b.obj + "_cgo_import.c"
b.add(Cmd{
- Args: []string{"cgo", "-dynimport", dynObj},
+ Args: {"cgo", "-dynimport", dynObj},
Stdout: importC,
- Input: []string{dynObj},
- Output: []string{importC},
+ Input: {dynObj},
+ Output: {importC},
})
b.script.addIntermediate(importC)

Index: src/pkg/html/parse.go
===================================================================
--- a/src/pkg/html/parse.go
+++ b/src/pkg/html/parse.go
@@ -1561,7 +1561,7 @@
func Parse(r io.Reader) (*Node, error) {
p := &parser{
tokenizer: NewTokenizer(r),
- doc: &Node{
+ doc: {
Type: DocumentNode,
},
scripting: true,
@@ -1581,7 +1581,7 @@
func ParseFragment(r io.Reader, context *Node) ([]*Node, error) {
p := &parser{
tokenizer: NewTokenizer(r),
- doc: &Node{
+ doc: {
Type: DocumentNode,
},
scripting: true,
Index: src/pkg/html/template/escape.go
===================================================================
--- a/src/pkg/html/template/escape.go
+++ b/src/pkg/html/template/escape.go
@@ -317,7 +317,7 @@
func newIdentCmd(identifier string) *parse.CommandNode {
return &parse.CommandNode{
NodeType: parse.NodeCommand,
- Args: []parse.Node{parse.NewIdentifier(identifier)},
+ Args: {parse.NewIdentifier(identifier)},
}
}

Index: src/pkg/math/big/rat.go
===================================================================
--- a/src/pkg/math/big/rat.go
+++ b/src/pkg/math/big/rat.go
@@ -136,7 +136,7 @@
// may change if a new value is assigned to x.
func (x *Rat) Denom() *Int {
if len(x.b) == 0 {
- return &Int{abs: nat{1}}
+ return &Int{abs: {1}}
}
return &Int{abs: x.b}
}
Index: src/pkg/old/netchan/export.go
===================================================================
--- a/src/pkg/old/netchan/export.go
+++ b/src/pkg/old/netchan/export.go
@@ -284,7 +284,7 @@
// NewExporter creates a new Exporter that exports a set of channels.
func NewExporter() *Exporter {
e := &Exporter{
- clientSet: &clientSet{
+ clientSet: {
names: make(map[string]*chanDir),
clients: make(map[unackedCounter]bool),
},
Index: src/pkg/testing/benchmark.go
===================================================================
--- a/src/pkg/testing/benchmark.go
+++ b/src/pkg/testing/benchmark.go
@@ -233,6 +233,6 @@
// Benchmark benchmarks a single function. Useful for creating
// custom benchmarks that do not use gotest.
func Benchmark(f func(b *B)) BenchmarkResult {
- b := &B{benchmark: InternalBenchmark{"", f}}
+ b := &B{benchmark: {"", f}}
return b.run()
}
Index: src/pkg/text/template/exec.go
===================================================================
--- a/src/pkg/text/template/exec.go
+++ b/src/pkg/text/template/exec.go
@@ -104,7 +104,7 @@
tmpl: t,
wr: wr,
line: 1,
- vars: []variable{{"$", value}},
+ vars: {{"$", value}},
}
if t.Tree == nil || t.Root == nil {
state.errorf("must be parsed before execution")

[r...@golang.org]

LGTM

http://codereview.appspot.com/5449071/

[da...@cheney.net]

LVGTM

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/ssh/transport.go#newcode224
src/pkg/exp/ssh/transport.go:224: reader: {
Excellent.

http://codereview.appspot.com/5449071/

[Robert Griesemer]

I may be the only one here, but I am not really liking these changes.
I am all for eliminating duplication of the same type name in the
slice outside and inside a literal, but these keyed struct fields now
become hard to read in my mind. I would not make these changes.
- gri

[Russ Cox]

On Fri, Dec 2, 2011 at 16:23, Robert Griesemer <g...@golang.org> wrote:
> I may be the only one here, but I am not really liking these changes.
> I am all for eliminating duplication of the same type name in the
> slice outside and inside a literal, but these keyed struct fields now
> become hard to read in my mind. I would not make these changes.

None of them?

[Brad Fitzpatrick]

Personally, I like fixing the ones were the type is obvious:

-               Args:   []string{"mkdir", "-p", name},

+               Args:   {"mkdir", "-p", name},

Or the ones where it's either stuttery and/or has really long type names (e.g. repeated slices in protos)

But if it's just one short type name and doesn't stutter, I like the readability of seeing the type.  But I might be more aggressive in eliding them in code I know well.  When reading other people's code, the types serve as reminders.  *shrug*

[g...@golang.org]

FYI.

http://codereview.appspot.com/5449071/diff/5001/src/pkg/archive/zip/writer.go
File src/pkg/archive/zip/writer.go (left):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/archive/zip/writer.go#oldcode35
src/pkg/archive/zip/writer.go:35: return &Writer{countWriter:
&countWriter{w: bufio.NewWriter(w)}}
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (left):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/encoding/xml/xml.go#oldcode1039
src/pkg/encoding/xml/xml.go:1039: R16: []unicode.Range16{
I'd leave this file alone - no real win

this is useful info and tiny compared to the size of the tables

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/norm/readwriter.go
File src/pkg/exp/norm/readwriter.go (left):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/norm/readwriter.go#oldcode67
src/pkg/exp/norm/readwriter.go:67: wr := &normWriter{rb:
reorderBuffer{}, w: w}
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/sql/sql.go
File src/pkg/exp/sql/sql.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/sql/sql.go#newcode198
src/pkg/exp/sql/sql.go:198: css: {{ci, si}},
definitively unreadable

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/exp/ssh/transport.go#newcode224
src/pkg/exp/ssh/transport.go:224: reader: {

no benefit in my mind

http://codereview.appspot.com/5449071/diff/5001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/go/build/build.go#newcode291
src/pkg/go/build/build.go:291: Args: {"mkdir", "-p", name},
I think these are fine because the elements are basic literals, and so
the type of the composite is clear ([]string, or perhaps []interface{}).

But I see that it's not the case below. I would probably leave the
[]string.

http://codereview.appspot.com/5449071/diff/5001/src/pkg/html/parse.go
File src/pkg/html/parse.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/html/parse.go#newcode1564
src/pkg/html/parse.go:1564: doc: {
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/html/template/escape.go
File src/pkg/html/template/escape.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/html/template/escape.go#newcode320
src/pkg/html/template/escape.go:320: Args:
{parse.NewIdentifier(identifier)},
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/math/big/rat.go
File src/pkg/math/big/rat.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/math/big/rat.go#newcode139
src/pkg/math/big/rat.go:139: return &Int{abs: {1}}
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/old/netchan/export.go
File src/pkg/old/netchan/export.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/old/netchan/export.go#newcode287
src/pkg/old/netchan/export.go:287: clientSet: {
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/testing/benchmark.go#newcode236
src/pkg/testing/benchmark.go:236: b := &B{benchmark: {"", f}}
I'd leave this file alone - no real win

http://codereview.appspot.com/5449071/diff/5001/src/pkg/text/template/exec.go
File src/pkg/text/template/exec.go (right):

http://codereview.appspot.com/5449071/diff/5001/src/pkg/text/template/exec.go#newcode107
src/pkg/text/template/exec.go:107: vars: {{"$", value}},
unreadable

http://codereview.appspot.com/5449071/

[Gustavo Niemeyer]

>> I may be the only one here, but I am not really liking these changes.
>> I am all for eliminating duplication of the same type name in the
>> slice outside and inside a literal, but these keyed struct fields now
>> become hard to read in my mind. I would not make these changes.
>
> None of them?

I've been going through the list too, separating readable from
unreadable entries. It seems very clear that there's one specific kind
of change that looks consistently bad: the ones involving struct
literals with field names. Those are things like:

- compCount: &countWriter{w: w},
+ compCount: {w: w},

The rest looks generally like a good improvement, with few exceptions.

--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[Robert Griesemer]

On Fri, Dec 2, 2011 at 3:11 PM, Gustavo Niemeyer <gus...@niemeyer.net> wrote:
> I've been going through the list too, separating readable from
> unreadable entries. It seems very clear that there's one specific kind
> of change that looks consistently bad: the ones involving struct
> literals with field names. Those are things like:
>
> -               compCount: &countWriter{w: w},
> +               compCount: {w: w},

+1

I am wondering if we really should permit this kind of shortcut at all
now. As a reader of somebody else's code, I am not convinced of the
benefit anymore.

- gri

[Gustavo Niemeyer]

On Fri, Dec 2, 2011 at 23:31, Robert Griesemer <g...@golang.org> wrote:
> +1
>
> I am wondering if we really should permit this kind of shortcut at all
> now. As a reader of somebody else's code, I am not convinced of the
> benefit anymore.

The only readable cases I've found where this was helpful is when the
field name was very close to the actual type name. E.g.:

URL: {Path: "/foo"}

This is fine. I'm not sure if that would be enough of a benefit to
justify the bad cases, though. There's certainly a difference between
reading

Top: {color: "red"},

and

Top: Banner{color: red},

The top isn't red.. the banner at the top is red.

Another problematic one regards the difference between slices and
structs. It used to be unambiguous, but now this:

Top: {"red", "blue"},

doesn't make clear if Top is taking struct with two fields, or is a
slice of strings. This could be relieved a bit by spelling implicit
slices as []{...} rather than {...}. So, e.g.:

Top: []{"red", "blue"},

[Gustavo Niemeyer]

> Another problematic one regards the difference between slices and
> structs. It used to be unambiguous, but now this:

Sorry, I realized this isn't entirely true. We already have the
possibility of saying:

type ColorPair []Color

or

type ColorPair struct{ Fore, Back Color }

Which means this:

Top: ColorPair{"red", "blue"},

is already ambiguous for the first time reader, so the point may be moot.

[Rob 'Commander' Pike]

This new ability allows code to be more concise but not always more readable. I'm bothered by our confusion on its merits.

The case for eliding types from structure literals is clearly less compelling than for slice and array literals, which is in part why it hasn't been pushed already. We see similar weirdnesses for slices of slices defined by profusions of anonymous braces.

So, I'm on the fence. On the one hand, concision can be convenient; on the other, it can be obscure. In some cases, such as protocol buffer literals, it could be a real boon, but the cost will always be less comprehensibility for a human reader.

Since this was intended as an experiment and misgivings are arising, I suggest we hold off for now. This sort of change is much harder to undo than to do, and holding off breaks no existing code. If we can find a stylistic convention or principle to guide us on when the readability cost of elision is offset by the code quality, we can revisit.

Further discussion is certainly welcome; I am just suggesting caution.

-rob

[Gustavo Niemeyer]

> This new ability allows code to be more concise but not always more readable. I'm bothered by our confusion on its merits.

The conversation feels somewhat expected so far. I personally can't
yet distinguish whether it's simply me being used to the status quo.

I wasn't around at the time, but it occurs to me that this is very
similar to implementing inference of typing on return values. I'm sure
someone complained about the code being less readable at the time too.

Observe the difference between:

x := page.Top()

and:

Page{
Top: {Color: v}
}

Both the lack of information and the availability of hints towards
what's the type, and even the way to figure what the type actually is,
are very analogous.

Are we uncomfortable simply because we're used to reading more? Is it
really a problem to look for the type definition as we do in function
calls, when we want to learn about the type?

Then, there are parts of the change that feel like easy wins because
there is still duplication of typing involved. E.g.:

[]*Page{{Top: r}, {Top: s}, {Top: t}}

This feels like an obvious and uncontroversial win.

[Andrew Gerrand]

On 3 December 2011 16:39, Rob 'Commander' Pike <r...@google.com> wrote:
> This new ability allows code to be more concise but not always more readable. I'm bothered by our confusion on its merits.

My reservation is that this change introduces another potential style
quibble. How many times are we going to see either "you should elide
the type here," or "please put the type name here," in code reviews?

One of the things I like about Go is that its syntax is simple so that
it doesn't let you make a lot of choices. This change seems like a
step back in that respect.

Andrew

[Rob 'Commander' Pike]

Agreed; that's why I said we may need stylistic rules to accept this change.

-rob