Issue 101410046: code review 101410046: gofmt/main: Added removal of empty declaration groups
|
Created:
9 years, 10 months ago by Simon Whitehead Modified:
9 years, 10 months ago Reviewers:
gri CC:
golang-codereviews, bradfitz, gri Visibility:
Public. |
Descriptiongofmt/main: Added removal of empty declaration groups.
Fixes issue 7631.
Patch Set 1 #Patch Set 2 : diff -r d628e9e34434 https://code.google.com/p/go #Patch Set 3 : diff -r d628e9e34434 https://code.google.com/p/go #
Total comments: 1
Patch Set 4 : diff -r 75040398cb4e https://code.google.com/p/go #Patch Set 5 : diff -r 75040398cb4e https://code.google.com/p/go #Patch Set 6 : diff -r 75040398cb4e https://code.google.com/p/go #
Total comments: 11
Patch Set 7 : diff -r baa0b6b4b248 https://code.google.com/p/go #
Total comments: 7
Patch Set 8 : diff -r 1fcd603852b0 https://code.google.com/p/go #
MessagesTotal messages: 15
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I think this should only be done if there are no adjacent or interior comments. Also, this needs tests. On Jun 23, 2014 6:50 AM, <chemNova@gmail.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > gofmt/main: Added removal of empty CONST, VAR and TYPE blocks. > > Fixes issue 7631. > > Please review this at https://codereview.appspot.com/101410046/ > > Affected files (+18, -0 lines): > M src/cmd/gofmt/simplify.go > > > Index: src/cmd/gofmt/simplify.go > =================================================================== > --- a/src/cmd/gofmt/simplify.go > +++ b/src/cmd/gofmt/simplify.go > @@ -117,5 +117,23 @@ > } > } > > + // remove empty CONST, VAR and TYPE blocks > + for i, d := range f.Decls { > + d, ok := d.(*ast.GenDecl) > + if !ok || (d.Tok != token.CONST && d.Tok != token.VAR && > d.Tok != token.TYPE) { > + continue > + } > + > + if !d.Lparen.IsValid() { > + // Not a block, skip > + continue > + } > + > + // if this is empty, remove it > + if d.Specs == nil { > + f.Decls = append(f.Decls[:i], f.Decls[i+1:]...) > + } > + } > + > ast.Walk(&s, f) > } > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >
Sign in to reply to this message.
This is too simplistic a solution. - This will lead to odd formatting if there are comments present in those groups (as bradfitz already pointed out). (Non-trivial). - This definitively requires tests. https://codereview.appspot.com/101410046/diff/40001/src/cmd/gofmt/simplify.go File src/cmd/gofmt/simplify.go (right): https://codereview.appspot.com/101410046/diff/40001/src/cmd/gofmt/simplify.go... src/cmd/gofmt/simplify.go:120: // remove empty CONST, VAR and TYPE blocks Why exclude imports? Also, they are not called blocks, but "groups".
Sign in to reply to this message.
On 2014/06/23 17:20:09, gri wrote: > This is too simplistic a solution. > > - This will lead to odd formatting if there are comments present in those groups > (as bradfitz already pointed out). (Non-trivial). > > - This definitively requires tests. > > https://codereview.appspot.com/101410046/diff/40001/src/cmd/gofmt/simplify.go > File src/cmd/gofmt/simplify.go (right): > > https://codereview.appspot.com/101410046/diff/40001/src/cmd/gofmt/simplify.go... > src/cmd/gofmt/simplify.go:120: // remove empty CONST, VAR and TYPE blocks > Why exclude imports? > > Also, they are not called blocks, but "groups". I appreciate the feedback. You could have both blasted me - but you were nice about it. Much appreciated on my first submission. I will check for comments, add tests and report back. Thanks again for being kind to a newcomer.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, bradfitz@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/06/24 05:50:31, SimonWhitehead wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:bradfitz@golang.org, mailto:gri@golang.org > (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look. This update contains: - Added IMPORT - Fixed logic - Tests - Corrected terminology from "blocks" to "groups"
Sign in to reply to this message.
This is much better (considering comments), but your code is way too complicated. Also, there's no need for so many test files - since it's easy to combine all tests into a single .input file w/o interference, please do that. https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.go File src/cmd/gofmt/simplify.go (right): https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:120: // Remove empty CONST, VAR, TYPE and IMPORT groups s/Remove/remove/ (in this package, no capitalization in comments that are not really full sentences) https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:126: func removeEmptyGroups(f *ast.File) { Some comments about this function below, but that said, please replace it completely by this much simpler function: func removeEmptyGroups(f *ast.File) { i := 0 for _, d := range f.Decls { if g, ok := d.(*ast.GenDecl); !ok || !isEmpty(f, g) { f.Decls[i] = d i++ } } f.Decls = f.Decls[:i] } write an isEmpty function that factors out the relevant tests https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:128: // Doing this to prevent broken iteration The 2nd commend line is useless - nobody knows what you mean by "broken iteration" (besides, it's not broken) https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:129: di := make([]int, 0) Instead of collecting the indices of declarations to remove, it's much simpler to collect the declarations you want to keep. https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:136: if !ok || (d.Tok != token.CONST && there are no other tokens in GenDecls - no need for the checks after the !ok https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:140: continue instead of using continue, factor these tests out into a helper function isEmpty and the code will look much nicer https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:150: if c.Pos() >= d.Pos() && c.End() <= d.End() { good, but it's nicer to write it d.Pos() <= c.Pos() && c.End() <= d.End() https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:162: // Loop through and remove each index This is _way_ too complicated!
Sign in to reply to this message.
Hi gri, Thank you so much for your feedback. I am quite embarrassed that I didn't refactor the code to be as straight-forward as you've put here. I definitely have some learning to do in regards to contributions. I will be fixing these up and removing the bloated tests. Again, thanks so much for being so kind in your reviews. I realise it must be frustrating for you! Cheers, Simon https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.go File src/cmd/gofmt/simplify.go (right): https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:126: func removeEmptyGroups(f *ast.File) { On 2014/06/25 18:47:38, gri wrote: > Some comments about this function below, but that said, please replace it > completely by this much simpler function: > > func removeEmptyGroups(f *ast.File) { > i := 0 > for _, d := range f.Decls { > if g, ok := d.(*ast.GenDecl); !ok || !isEmpty(f, g) { > f.Decls[i] = d > i++ > } > } > f.Decls = f.Decls[:i] > } > > write an isEmpty function that factors out the relevant tests This is fantastic. I am a bit embarrassed that my solution wasn't refactored into this. Thank you very much. I will implement this. https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:128: // Doing this to prevent broken iteration On 2014/06/25 18:47:39, gri wrote: > The 2nd commend line is useless - nobody knows what you mean by "broken > iteration" (besides, it's not broken) Agreed - unfortunately I left comments in there that make sense to just me. I apologise. https://codereview.appspot.com/101410046/diff/100001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:150: if c.Pos() >= d.Pos() && c.End() <= d.End() { On 2014/06/25 18:47:38, gri wrote: > good, but it's nicer to write it > > d.Pos() <= c.Pos() && c.End() <= d.End() Again - fantastic. Thanks so much. Implemented.
Sign in to reply to this message.
Much better! Some more suggestions. In the worst-case, this algorithm is quadratic, because for each empty declaration, all comments are searched. That said, it's such a rare case that we have an empty declaration, and iterating through all comments is so fast, that I wouldn't worry about it. Especially if you don't enter the loop when not needed, per my suggestion. https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.go File src/cmd/gofmt/simplify.go (right): https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:120: // remove empty CONST, VAR, TYPE and IMPORT groups I'd leave away this comment or change it - these are the only declarations that can be grouped anyway. You could: - chose a more explicit function name instead, say: removeEmptyDeclGroups - make the comment more concrete using an example (this will make it much clearer for readers w/o having to spell out every possibility): // remove empty declarations such as "const ()", etc. https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:137: func isEmpty(f *ast.File, g *ast.GenDecl) (e bool) { remove name of result parameter - not needed https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:138: e = g.Doc == nil && g.Specs == nil There's no need to go into the range loop (and iterate over all comments in the file) if you have a doc string or if the group is not empty. Why not be explicit: if g.Doc != nil || g.Specs != nil { return false } Alternative: if g.Doc == nil && g.Specs == nil { // for loop } But the first suggestion is more explicit and immediately removes a concern for a reader trying to understand the code because we're just returning. It also leads to less indentation. https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:140: for _, c := range f.Comments { please add a comment // if there's a comment in the declaration, it is not considered empty https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:142: e = false return false no need to keep iterating - it's not going to change anymore https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/simplify.g... src/cmd/gofmt/simplify.go:146: return return true https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/testdata/e... File src/cmd/gofmt/testdata/emptydecl.input (right): https://codereview.appspot.com/101410046/diff/120001/src/cmd/gofmt/testdata/e... src/cmd/gofmt/testdata/emptydecl.input:3: // Keep I think what you had before was fine - no need to be super-frugal // keep this declaration (same below)
Sign in to reply to this message.
PS: Please also change the CL description - empty Imports are also removed. Again, I'd just say that this removes empty declaration groups. On Thu, Jun 26, 2014 at 4:32 PM, <gri@golang.org> wrote: > Much better! > > Some more suggestions. > > In the worst-case, this algorithm is quadratic, because for each empty > declaration, all comments are searched. That said, it's such a rare case > that we have an empty declaration, and iterating through all comments is > so fast, that I wouldn't worry about it. Especially if you don't enter > the loop when not needed, per my suggestion. > > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go > File src/cmd/gofmt/simplify.go (right): > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode120 > > src/cmd/gofmt/simplify.go:120: // remove empty CONST, VAR, TYPE and > IMPORT groups > I'd leave away this comment or change it - these are the only > declarations that can be grouped anyway. You could: > > - chose a more explicit function name instead, say: > removeEmptyDeclGroups > > - make the comment more concrete using an example (this will make it > much clearer for readers w/o having to spell out every possibility): > > // remove empty declarations such as "const ()", etc. > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode137 > src/cmd/gofmt/simplify.go:137: func isEmpty(f *ast.File, g *ast.GenDecl) > (e bool) { > remove name of result parameter - not needed > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode138 > src/cmd/gofmt/simplify.go:138: e = g.Doc == nil && g.Specs == nil > There's no need to go into the range loop (and iterate over all comments > in the file) if you have a doc string or if the group is not empty. Why > not be explicit: > > if g.Doc != nil || g.Specs != nil { > return false > } > > Alternative: > > if g.Doc == nil && g.Specs == nil { > // for loop > } > > But the first suggestion is more explicit and immediately removes a > concern for a reader trying to understand the code because we're just > returning. It also leads to less indentation. > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode140 > src/cmd/gofmt/simplify.go:140: for _, c := range f.Comments { > please add a comment > > // if there's a comment in the declaration, it is not considered empty > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode142 > src/cmd/gofmt/simplify.go:142: e = false > return false > > no need to keep iterating - it's not going to change anymore > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/simplify.go#newcode146 > src/cmd/gofmt/simplify.go:146: return > return true > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/testdata/emptydecl.input > File src/cmd/gofmt/testdata/emptydecl.input (right): > > https://codereview.appspot.com/101410046/diff/120001/src/ > cmd/gofmt/testdata/emptydecl.input#newcode3 > src/cmd/gofmt/testdata/emptydecl.input:3: // Keep > I think what you had before was fine - no need to be super-frugal > > // keep this declaration > > (same below) > > https://codereview.appspot.com/101410046/ >
Sign in to reply to this message.
On 2014/06/26 23:36:16, gri wrote: > PS: Please also change the CL description - empty Imports are also removed. > Again, I'd just say that this removes empty declaration groups. > > > On Thu, Jun 26, 2014 at 4:32 PM, <mailto:gri@golang.org> wrote: > > > Much better! > > > > Some more suggestions. > > > > In the worst-case, this algorithm is quadratic, because for each empty > > declaration, all comments are searched. That said, it's such a rare case > > that we have an empty declaration, and iterating through all comments is > > so fast, that I wouldn't worry about it. Especially if you don't enter > > the loop when not needed, per my suggestion. > > > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go > > File src/cmd/gofmt/simplify.go (right): > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode120 > > > > src/cmd/gofmt/simplify.go:120: // remove empty CONST, VAR, TYPE and > > IMPORT groups > > I'd leave away this comment or change it - these are the only > > declarations that can be grouped anyway. You could: > > > > - chose a more explicit function name instead, say: > > removeEmptyDeclGroups > > > > - make the comment more concrete using an example (this will make it > > much clearer for readers w/o having to spell out every possibility): > > > > // remove empty declarations such as "const ()", etc. > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode137 > > src/cmd/gofmt/simplify.go:137: func isEmpty(f *ast.File, g *ast.GenDecl) > > (e bool) { > > remove name of result parameter - not needed > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode138 > > src/cmd/gofmt/simplify.go:138: e = g.Doc == nil && g.Specs == nil > > There's no need to go into the range loop (and iterate over all comments > > in the file) if you have a doc string or if the group is not empty. Why > > not be explicit: > > > > if g.Doc != nil || g.Specs != nil { > > return false > > } > > > > Alternative: > > > > if g.Doc == nil && g.Specs == nil { > > // for loop > > } > > > > But the first suggestion is more explicit and immediately removes a > > concern for a reader trying to understand the code because we're just > > returning. It also leads to less indentation. > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode140 > > src/cmd/gofmt/simplify.go:140: for _, c := range f.Comments { > > please add a comment > > > > // if there's a comment in the declaration, it is not considered empty > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode142 > > src/cmd/gofmt/simplify.go:142: e = false > > return false > > > > no need to keep iterating - it's not going to change anymore > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/simplify.go#newcode146 > > src/cmd/gofmt/simplify.go:146: return > > return true > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/testdata/emptydecl.input > > File src/cmd/gofmt/testdata/emptydecl.input (right): > > > > https://codereview.appspot.com/101410046/diff/120001/src/ > > cmd/gofmt/testdata/emptydecl.input#newcode3 > > src/cmd/gofmt/testdata/emptydecl.input:3: // Keep > > I think what you had before was fine - no need to be super-frugal > > > > // keep this declaration > > > > (same below) > > > > https://codereview.appspot.com/101410046/ > > Many thanks Robert. I have implemented your suggestions.
Sign in to reply to this message.
LGTM Thanks for doing this. Please sign the CLA ( https://developers.google.com/open-source/cla/individual ) per the instructions http://golang.org/doc/contribute.html#copyright . I cannot submit your changes otherwise. - gri
Sign in to reply to this message.
Hi Robert, All signed. It says it will be "processed shortly". Thank you so much for holding my hand through this. I feel it has been above and beyond what is asked of reviewers. I have learnt a lot about contributing and will endeavour to make future contributions less painful for reviewers. Many thanks again, Simon On 2014/06/30 17:11:44, gri wrote: > LGTM > > Thanks for doing this. > > Please sign the CLA ( https://developers.google.com/open-source/cla/individual ) > per the instructions http://golang.org/doc/contribute.html#copyright . I cannot > submit your changes otherwise. > > - gri
Sign in to reply to this message.
I've processed the CLA. Robert can proceed with the review. On Mon, Jun 30, 2014 at 3:49 PM, <chemNova@gmail.com> wrote: > Hi Robert, > > All signed. It says it will be "processed shortly". > > Thank you so much for holding my hand through this. I feel it has been > above and beyond what is asked of reviewers. > > I have learnt a lot about contributing and will endeavour to make future > contributions less painful for reviewers. > > Many thanks again, > > Simon > > On 2014/06/30 17:11:44, gri wrote: > >> LGTM >> > > Thanks for doing this. >> > > Please sign the CLA ( >> > https://developers.google.com/open-source/cla/individual ) > >> per the instructions http://golang.org/doc/contribute.html#copyright . >> > I cannot > >> submit your changes otherwise. >> > > - gri >> > > > > https://codereview.appspot.com/101410046/ >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=43e061c6564c *** gofmt/main: Added removal of empty declaration groups. Fixes issue 7631. LGTM=gri R=golang-codereviews, bradfitz, gri CC=golang-codereviews https://codereview.appspot.com/101410046 Committer: Robert Griesemer <gri@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
