Issue 3225042: code review 3225042: go/printer: preserve newlines in func parameter lists
Descriptiongo/printer: preserve newlines in func parameter lists
Fixes issue 1179.
Patch Set 1 #
Total comments: 20
Patch Set 2 : code review 3225042: go/printer: preserve newlines in func parameter lists #
MessagesTotal messages: 6
Thanks for doing this. Looks pretty good. Please add the line: Fixes issue 1179. to the CL description; this will close the bug upon submission. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go File src/pkg/go/printer/nodes.go (right): http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:301: var prev, next token.Position Please use "var prevLine, line int" instead - it's in sync with the code in exprList. Also, it will be more efficient, especially with the new token.Pos implementation (forthcoming). http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:305: next = par.Names[0].Pos() this will become: line = par.Names[0].Pos().Line http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:307: next = par.Type.Pos() analogous http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:309: p.print(token.COMMA) I'd move this before the computation of next/line - it always needs to be printed. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:310: if prev.IsValid() && prev.Line < next.Line && This line would become: if prevLine < line && prevLine > 0 && line > 0 { (see nodes.go:247) http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:311: p.linebreak(next.Line, 0, ignore, true) { I am not sure about the true here. If you have comments interspersed, they will not line up correctly; but they probably should. Try using false and add more test cases. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:322: prev = par.Type.Pos() prevLine = par.Type.Pos().Line http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... File src/pkg/go/printer/testdata/declarations.golden (right): http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:659: please two newlines between groups of tests http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:662: int) { I am wondering if there should be indentation here. Something one can fine-tune later. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:669: } Please add more examples that test the behavior if comments are following groups of equally-typed parameters.
Sign in to reply to this message.
http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go File src/pkg/go/printer/nodes.go (right): http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:301: var prev, next token.Position On 2010/12/01 22:41:34, gri wrote: > Please use "var prevLine, line int" instead - it's in sync with the code in > exprList. Also, it will be more efficient, especially with the new token.Pos > implementation (forthcoming). Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:305: next = par.Names[0].Pos() On 2010/12/01 22:41:34, gri wrote: > this will become: > > line = par.Names[0].Pos().Line Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:307: next = par.Type.Pos() On 2010/12/01 22:41:34, gri wrote: > analogous Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:309: p.print(token.COMMA) On 2010/12/01 22:41:34, gri wrote: > I'd move this before the computation of next/line - it always needs to be > printed. Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:310: if prev.IsValid() && prev.Line < next.Line && On 2010/12/01 22:41:34, gri wrote: > This line would become: > > if prevLine < line && prevLine > 0 && line > 0 { > > (see nodes.go:247) I went with '0 < prevLine && prevLine < line', as that implies 0 < line. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:311: p.linebreak(next.Line, 0, ignore, true) { On 2010/12/01 22:41:34, gri wrote: > I am not sure about the true here. If you have comments interspersed, they will > not line up correctly; but they probably should. Try using false and add more > test cases. I've added the additional test cases, but passing true or false for the newSection param of linebreak doesn't seem to have any effect on this. I also added test cases with a comment line in between the params, and still saw no difference between the values of newSection. Can you describe what formatting you're looking for with the comments interspersed? Also, I should point out that (IIRC) I based this linebreak call on the one in exprList on line 190 in this file. I believe that linebreak is responsible for preserving the newlines in between two param names. This one only applies to a newline between a type name followed by a param name. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... src/pkg/go/printer/nodes.go:322: prev = par.Type.Pos() On 2010/12/01 22:41:34, gri wrote: > prevLine = par.Type.Pos().Line Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... File src/pkg/go/printer/testdata/declarations.golden (right): http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:659: On 2010/12/01 22:41:34, gri wrote: > please two newlines between groups of tests Done. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:662: int) { On 2010/12/01 22:41:34, gri wrote: > I am wondering if there should be indentation here. Something one can fine-tune > later. This matches the existing behavior for func param lists with a \n in between two param names rather than after a type name. The last test case in this revision of the change was already passing before I modified nodes.go. I'm happy to make the change, but I've actually grown to like the lack of indentation there. I find that it makes it more clear where the func body starts. http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... src/pkg/go/printer/testdata/declarations.golden:669: } On 2010/12/01 22:41:34, gri wrote: > Please add more examples that test the behavior if comments are following groups > of equally-typed parameters. I've added a number of additional test cases with comments in various places while trying to identify the difference in behavior of the newSection param of linebreak. All the test cases pass regardless of that param. Please let me know which of the test cases you think should be kept.
Sign in to reply to this message.
Hello gri (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Sign in to reply to this message.
On Thu, Dec 2, 2010 at 9:35 AM, <jgennis@google.com> wrote: > Reviewers: gri, > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go > File src/pkg/go/printer/nodes.go (right): > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:301: var prev, next token.Position > On 2010/12/01 22:41:34, gri wrote: > >> Please use "var prevLine, line int" instead - it's in sync with the >> > code in > >> exprList. Also, it will be more efficient, especially with the new >> > token.Pos > >> implementation (forthcoming). >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:305: next = par.Names[0].Pos() > On 2010/12/01 22:41:34, gri wrote: > >> this will become: >> > > line = par.Names[0].Pos().Line >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:307: next = par.Type.Pos() > On 2010/12/01 22:41:34, gri wrote: > >> analogous >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:309: p.print(token.COMMA) > On 2010/12/01 22:41:34, gri wrote: > >> I'd move this before the computation of next/line - it always needs to >> > be > >> printed. >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:310: if prev.IsValid() && prev.Line < > next.Line && > On 2010/12/01 22:41:34, gri wrote: > >> This line would become: >> > > if prevLine < line && prevLine > 0 && line > 0 { >> > > (see nodes.go:247) >> > > I went with '0 < prevLine && prevLine < line', as that implies 0 < line. sounds good > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:311: p.linebreak(next.Line, 0, ignore, true) > { > On 2010/12/01 22:41:34, gri wrote: > >> I am not sure about the true here. If you have comments interspersed, >> > they will > >> not line up correctly; but they probably should. Try using false and >> > add more > >> test cases. >> > > I've added the additional test cases, but passing true or false for the > newSection param of linebreak doesn't seem to have any effect on this. I > also added test cases with a comment line in between the params, and > still saw no difference between the values of newSection. > I see that. It would be nice if the two comments on line 705, 709 (declarations.golden) lined up, but I think that is related to other code. I will look into that some other time. > > Can you describe what formatting you're looking for with the comments > interspersed? > comments on consecutive lines should line up - again, I think now this is not related to this particular code > > Also, I should point out that (IIRC) I based this linebreak call on the > one in exprList on line 190 in this file. I believe that linebreak is > responsible for preserving the newlines in between two param names. > This one only applies to a newline between a type name followed by a > param name. yes > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/nodes.go#newc... > src/pkg/go/printer/nodes.go:322: prev = par.Type.Pos() > On 2010/12/01 22:41:34, gri wrote: > >> prevLine = par.Type.Pos().Line >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... > File src/pkg/go/printer/testdata/declarations.golden (right): > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... > src/pkg/go/printer/testdata/declarations.golden:659: > On 2010/12/01 22:41:34, gri wrote: > >> please two newlines between groups of tests >> > > Done. > > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... > src/pkg/go/printer/testdata/declarations.golden:662: int) { > On 2010/12/01 22:41:34, gri wrote: > >> I am wondering if there should be indentation here. Something one can >> > fine-tune > >> later. >> > > This matches the existing behavior for func param lists with a \n in > between two param names rather than after a type name. The last test > case in this revision of the change was already passing before I > modified nodes.go. > > I'm happy to make the change, but I've actually grown to like the lack > of indentation there. I find that it makes it more clear where the func > body starts. > > fine for now > > > http://codereview.appspot.com/3225042/diff/1/src/pkg/go/printer/testdata/decl... > src/pkg/go/printer/testdata/declarations.golden:669: } > On 2010/12/01 22:41:34, gri wrote: > >> Please add more examples that test the behavior if comments are >> > following groups > >> of equally-typed parameters. >> > > I've added a number of additional test cases with comments in various > places while trying to identify the difference in behavior of the > newSection param of linebreak. All the test cases pass regardless of > that param. > > Please let me know which of the test cases you think should be kept. > let's leave them all, it's good to have them systematically enumerated > > Description: > go/printer: preserve newlines in func parameter lists > > Fixes issue 1179. > > Please review this at http://codereview.appspot.com/3225042/ > > Affected files: > M src/pkg/go/printer/nodes.go > M src/pkg/go/printer/testdata/declarations.golden > M src/pkg/go/printer/testdata/declarations.input > > > Index: src/pkg/go/printer/nodes.go > =================================================================== > --- a/src/pkg/go/printer/nodes.go > +++ b/src/pkg/go/printer/nodes.go > @@ -92,7 +92,7 @@ > > > // Sets multiLine to true if the identifier list spans multiple lines. > -// If ident is set, a multi-line identifier list is indented after the > +// If indent is set, a multi-line identifier list is indented after the > // first linebreak encountered. > func (p *printer) identList(list []*ast.Ident, indent bool, multiLine > *bool) { > // convert into an expression list so we can re-use exprList > formatting > @@ -298,15 +298,28 @@ > func (p *printer) parameters(fields *ast.FieldList, multiLine *bool) { > p.print(fields.Opening, token.LPAREN) > if len(fields.List) > 0 { > > + var prev, next token.Position > for i, par := range fields.List { > if i > 0 { > - p.print(token.COMMA, blank) > + if len(par.Names) > 0 { > > + next = par.Names[0].Pos() > + } else { > + next = par.Type.Pos() > + } > + p.print(token.COMMA) > > + if prev.IsValid() && prev.Line < next.Line > && > + p.linebreak(next.Line, 0, ignore, > true) { > + *multiLine = true > + } else { > + p.print(blank) > + } > } > if len(par.Names) > 0 { > p.identList(par.Names, false, multiLine) > p.print(blank) > } > p.expr(par.Type, multiLine) > + prev = par.Type.Pos() > } > } > p.print(fields.Closing, token.RPAREN) > Index: src/pkg/go/printer/testdata/declarations.golden > =================================================================== > --- a/src/pkg/go/printer/testdata/declarations.golden > +++ b/src/pkg/go/printer/testdata/declarations.golden > @@ -656,3 +656,14 @@ > func _(x ...func(...int)) > func _(x ...map[string]int) > func _(x ...chan int) > + > +// these parameter lists must remain multi-line since they are multi-line > in the source > +func _(bool, > +int) { > +} > +func _(x bool, > +y int) { > +} > +func _(x, > +y bool) { > +} > Index: src/pkg/go/printer/testdata/declarations.input > =================================================================== > --- a/src/pkg/go/printer/testdata/declarations.input > +++ b/src/pkg/go/printer/testdata/declarations.input > @@ -644,3 +644,14 @@ > func _(x ...func(...int)) > func _(x ...map[string]int) > func _(x ...chan int) > + > +// these parameter lists must remain multi-line since they are multi-line > in the source > +func _(bool, > +int) { > +} > +func _(x bool, > +y int) { > +} > +func _(x, > +y bool) { > +} > > >
Sign in to reply to this message.
LGTM thanks for doing this - gri On Thu, Dec 2, 2010 at 9:35 AM, <jgennis@google.com> wrote: > Hello gri (cc: golang-dev@googlegroups.com), > > I'd like you to review this change. > > > http://codereview.appspot.com/3225042/ >
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=32bccfd99223 *** go/printer: preserve newlines in func parameter lists Fixes issue 1179. R=gri CC=golang-dev http://codereview.appspot.com/3225042 Committer: Robert Griesemer <gri@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
