code review 802043: Gofmt preserves newlines in multiline selector expressions.

Gofmt preserves newlines in multiline selector expressions.

This is for making the fluent interface idiom usable with gofmt.

[rsaarelm, 2010-03-27 13:33:26]

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

I'd like you to review this change.

[rsaarelm, 2010-03-27 16:24:51]

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

Please take another look.

[gri, 2010-03-29 22:06:00]

Thanks for looking into this. I think this looks essentially right, but I think
the logic can be simplified and made clearer; there may be some missing cases,
and there are some nitpicks to match the rest of the style in this file.

Also, please fill in the license form otherwise I won't be able to submit this
under your name once it is ready to go. See:
http://golang.org/doc/contribute.html .

http://codereview.appspot.com/802043/diff/5002/6001
File src/pkg/go/printer/nodes.go (right):

http://codereview.appspot.com/802043/diff/5002/6001#newcode13
src/pkg/go/printer/nodes.go:13: "container/list"
don't use a list, please use a vector

(List is not used elsewhere in gofmt, but vector is - no need to introduce an
extra dependency.)

http://codereview.appspot.com/802043/diff/5002/6001#newcode93
src/pkg/go/printer/nodes.go:93: periodSep                           // elements
are separated by periods
good!

http://codereview.appspot.com/802043/diff/5002/6001#newcode260
src/pkg/go/printer/nodes.go:260: } else if mode&periodSep != 0 {
invert logic and get rid of this branch

http://codereview.appspot.com/802043/diff/5002/6001#newcode263
src/pkg/go/printer/nodes.go:263: } else {
after inverting the logic, comment here:

// period-separated list elements don't need a blank

http://codereview.appspot.com/802043/diff/5002/6001#newcode673
src/pkg/go/printer/nodes.go:673: 
two empty lines between functions please (prevalent style in this file)

http://codereview.appspot.com/802043/diff/5002/6001#newcode676
src/pkg/go/printer/nodes.go:676: // selector isn't found. Rewrite AST nodes for
calls, indexings and type
s/Rewrite/Rewrites/

http://codereview.appspot.com/802043/diff/5002/6001#newcode676
src/pkg/go/printer/nodes.go:676: // selector isn't found. Rewrite AST nodes for
calls, indexings and type
s/indexings/index expressions/

http://codereview.appspot.com/802043/diff/5002/6001#newcode677
src/pkg/go/printer/nodes.go:677: // assertions, all of which you may get in
selector chains, to make them parts
s/you/one/

http://codereview.appspot.com/802043/diff/5002/6001#newcode679
src/pkg/go/printer/nodes.go:679: func splitSelector(expr ast.Expr) (body, suffix
ast.Expr, hasSelector bool) {
This can be written more elegantly: There is no need for the hasSelector result
because body is nil if there's no selector. Sufficient to simply return body and
suffix, replace all checks for hasSelector into body != nil, and get rid of
hasSelector.

http://codereview.appspot.com/802043/diff/5002/6001#newcode684
src/pkg/go/printer/nodes.go:684: body = x.X
perhaps write this as

body, suffix := x.X, x.Sel

this will match the incantation in the other cases

http://codereview.appspot.com/802043/diff/5002/6001#newcode699
src/pkg/go/printer/nodes.go:699: }
What about the other possibilities? SliceExpr, TypeAssertions? Are the more?

http://codereview.appspot.com/802043/diff/5002/6001#newcode708
src/pkg/go/printer/nodes.go:708: collect := list.New()
change to:

var list vector.Vector

http://codereview.appspot.com/802043/diff/5002/6001#newcode709
src/pkg/go/printer/nodes.go:709: body, suffix, hasSel := splitSelector(expr)
With the suggested change avobe (getting rid of hasSel result, this for loop can
be written much more elegantly as:

	var list vector.Vector
	for expr != nil {
		var suffix ast.Expr
		expr, suffix = splitSelector(expr)
		list.Push(suffix)
	}

http://codereview.appspot.com/802043/diff/5002/6001#newcode719
src/pkg/go/printer/nodes.go:719: result = make([]ast.Expr, collect.Len())
this becomes:

	result = make([]ast.Expr, len(list))
	i := len(list)
	for _, x := range list {
		i--
		result[i] = x.(ast.Expr)
	}

http://codereview.appspot.com/802043/diff/5002/6001#newcode727
src/pkg/go/printer/nodes.go:727: 
two empty lines between functions please (prevalent style in this file)

http://codereview.appspot.com/802043/diff/5002/6003
File src/pkg/go/printer/testdata/expressions.input (right):

http://codereview.appspot.com/802043/diff/5002/6003#newcode403
src/pkg/go/printer/testdata/expressions.input:403: func _() {
Some of the tests in this file should be a bit misformated so that one can see
the effect of the formatting.

http://codereview.appspot.com/802043/diff/5002/6003#newcode411
src/pkg/go/printer/testdata/expressions.input:411: foo(2). // Inline comments.
minor: keep line comments all lower-case w/o terminating periods - to keep in
style with the rest of this file

http://codereview.appspot.com/802043/diff/5002/6003#newcode434
src/pkg/go/printer/testdata/expressions.input:434: 4].
should have test cases with slices and type assertions

[rsaarelm, 2010-03-30 15:12:52]

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

Please take another look.

[gri, 2010-03-30 18:39:33]

LGTM
Thanks for doing this.

(There's a couple of minor comment issues; I will submit as is and fix those
myself to save round-trip time.)

http://codereview.appspot.com/802043/diff/11001/12001
File src/pkg/go/printer/nodes.go (right):

http://codereview.appspot.com/802043/diff/11001/12001#newcode263
src/pkg/go/printer/nodes.go:263: // period-separadet list elements don't need a
blank
s/separadet/separated/

http://codereview.appspot.com/802043/diff/11001/12001#newcode679
src/pkg/go/printer/nodes.go:679: // Rewrite call and index expressions to be a
part of the selector chain so
this comment is not needed - mention already in the function comment

[gri, 2010-03-30 18:46:24]

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

Gofmt preserves newlines in multiline selector expressions.

This is for making the fluent interface idiom usable with gofmt.

R=gri
CC=golang-dev
http://codereview.appspot.com/802043

Committer: Robert Griesemer <gri@golang.org>