code review 5449071: various: shorten composite literal field values

various: shorten composite literal field values

[rsc, 2011-12-02 20:01:22]

Hello golang-dev@googlegroups.com,

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

[r, 2011-12-02 20:47:15]

LGTM

[dave_cheney.net, 2011-12-02 20:49:41]

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#...
src/pkg/exp/ssh/transport.go:224: reader: {
Excellent.

[gri, 2011-12-02 21:23:25]

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

On Fri, Dec 2, 2011 at 12:49 PM,  <dave@cheney.net> wrote:
> 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#...
> src/pkg/exp/ssh/transport.go:224: reader: {
> Excellent.
>
> http://codereview.appspot.com/5449071/

[rsc, 2011-12-02 21:24:48]

On Fri, Dec 2, 2011 at 16:23, Robert Griesemer <gri@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?

[bradfitz, 2011-12-02 21:53:10]

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*


On Fri, Dec 2, 2011 at 1:24 PM, Russ Cox <rsc@golang.org> wrote:

> On Fri, Dec 2, 2011 at 16:23, Robert Griesemer <gri@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?
>

[gri, 2011-12-02 23:04:18]

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...
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#o...
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.g...
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#newcod...
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#...
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#new...
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#newcode...
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....
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#newco...
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...
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#...
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...
src/pkg/text/template/exec.go:107: vars: {{"$", value}},
unreadable

[gustavo_niemeyer.net, 2011-12-02 23:11:46]

>> 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.

[gri, 2011-12-03 01:31:50]

On Fri, Dec 2, 2011 at 3:11 PM, Gustavo Niemeyer <gustavo@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.net, 2011-12-03 02:32:00]

On Fri, Dec 2, 2011 at 23:31, Robert Griesemer <gri@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
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[gustavo_niemeyer.net, 2011-12-03 02:40:44]

> 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.

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

-- I'm not absolutely sure of anything.

[r2, 2011-12-03 05:39:15]

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.net, 2011-12-03 14:11:13]

> 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.

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

-- I'm not absolutely sure of anything.

[adg, 2011-12-03 21:56:59]

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

[r2, 2011-12-03 22:00:08]

On Dec 3, 2011, at 1:56 PM, Andrew Gerrand wrote:

> 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.

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

-rob

[rsc, 2011-12-05 19:03:58]

*** Abandoned ***