code review 12837044: cmd/gofmt: sort more, remove some duplicate imports

cmd/gofmt: sort more, remove some duplicate imports

* Sort imports by import path, then import name, then comment. Currently, gofmt sorts only by import path.
* If two imports have the same import path and import name, and one of them has no comment, remove the import with no comment. (See the discussion at issue 4414.)

Based on @rsc's https://codereview.appspot.com/7231070/

Fixes issue 4414.

[josharian, 2013-08-27 18:05:45]

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

I'd like you to review this change to
https://code.google.com/p/go

[josharian, 2013-08-27 18:07:14]

On 2013/08/27 18:05:45, josharian wrote:
> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Sorry it took me so long to get back to this. It's (obviously) fine if you want
to delay this past Go 1.2; I know that it is crunch time now.

Thanks,
Josh

[rsc, 2013-09-06 20:15:03]

LGTM

I hope that when gri is back he will comment on token.(*File).RemoveLine, but
for now it seems fine.

[rsc, 2013-09-06 20:25:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=f09e38f4b079 ***

cmd/gofmt: sort more, remove some duplicate imports

* Sort imports by import path, then import name, then comment. Currently, gofmt
sorts only by import path.
* If two imports have the same import path and import name, and one of them has
no comment, remove the import with no comment. (See the discussion at issue
4414.)

Based on @rsc's https://codereview.appspot.com/7231070/

Fixes issue 4414.

R=gri, rsc
CC=golang-dev
https://codereview.appspot.com/12837044

Committer: Russ Cox <rsc@golang.org>

[gri, 2013-09-10 20:51:05]

https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position....
src/pkg/go/token/position.go:139: // RemoveLine removes a line by line number as
reported by Position.Line.
RemoveLine is slightly misleading a name.

This function simply removes a line offset from the table, which is akin to
replacing a newline character (causing the new line) with a space (to not change
the remaining offsets).

This is probably the desired effect for the specific application (we don't want
to re-adjust all positions), but perhaps the function should be named more
aptly? MergeLines comes to mind since this merges two lines - but doesn't really
change the subsequent position offsets. The comments should be adjusted as well.

Also, lines are numbered starting at 1 while the table is 0-indexed; there seems
to be an off-by-1 error somewhere; or the comment needs to be adjusted. An input
of 0 should not be accepted, either.

[rsc, 2013-09-10 20:57:48]

Can you send a CL?

Thanks.
Russ

[josharian, 2013-09-10 23:34:49]

rsc wrote: "Can you send a CL?"

I'm not sure whether "you" refers to me or gri, but I'd be happy to send a
cleanup CL. The only outstanding question, to my mind, is the proper name of the
function; see my inline reply. I'd love input.

Thanks for the careful review, gri, much appreciated.

-josh

https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position....
src/pkg/go/token/position.go:139: // RemoveLine removes a line by line number as
reported by Position.Line.
> RemoveLine is slightly misleading a name.

Agreed. I started with RemoveNewline, was unsure about its clarity, and failed
to come up with a better alternative.


> MergeLines comes to mind since this merges two lines - but doesn't really
> change the subsequent position offsets.

I thought about MergeLines, but that feels like it should take two parameters.
Other suggestions?


> The comments should be adjusted as well.

Yes.


> Also, lines are numbered starting at 1 while the table is 0-indexed; there
seems
> to be an off-by-1 error somewhere; or the comment needs to be adjusted. An
input
> of 0 should not be accepted, either.

When I looked at this, the first entry in the table was always 0; in practice,
the code below is (I believe) correct and does match the doc. I assumed that
that had been done to avoid having to think about off-by-1 errors when
manipulating the table, and I appreciated it at the time!

[gri, 2013-09-10 23:45:24]

I think rsc meant me, but please feel free to take a crack at it. Please
send the review directly to me.

I think MergeLines with one argument is fine. Or MergeLine (singular), as
in merge this line with the next one. Or JoinLine. We can debate a better
name if one comes up.

The table is defined to contain the offset of the first character for each
line, so the first entry is always 0 as you observed. I'm not saying the
code is wrong, but by the comment, valid input line numbers should start at
1 (currently, 0 would not crash, I think). Maybe that requires a +1, -1
that cancel each other out, but at least then then we have a match between
API documentation and behavior.

Hope that helps.
- gri



On Tue, Sep 10, 2013 at 4:34 PM, <josharian@gmail.com> wrote:

> rsc wrote: "Can you send a CL?"
>
> I'm not sure whether "you" refers to me or gri, but I'd be happy to send
> a cleanup CL. The only outstanding question, to my mind, is the proper
> name of the function; see my inline reply. I'd love input.
>
> Thanks for the careful review, gri, much appreciated.
>
> -josh
>
>
>
>
> https://codereview.appspot.**com/12837044/diff/13001/src/**
>
pkg/go/token/position.go<https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go>
> File src/pkg/go/token/position.go (right):
>
> https://codereview.appspot.**com/12837044/diff/13001/src/**
>
pkg/go/token/position.go#**newcode139<https://codereview.appspot.com/12837044/diff/13001/src/pkg/go/token/position.go#newcode139>
> src/pkg/go/token/position.go:**139: // RemoveLine removes a line by line
> number as reported by Position.Line.
>
>> RemoveLine is slightly misleading a name.
>>
>
> Agreed. I started with RemoveNewline, was unsure about its clarity, and
> failed to come up with a better alternative.
>
>
>
>  MergeLines comes to mind since this merges two lines - but doesn't
>>
> really
>
>> change the subsequent position offsets.
>>
>
> I thought about MergeLines, but that feels like it should take two
> parameters. Other suggestions?
>
>
>
>  The comments should be adjusted as well.
>>
>
> Yes.
>
>
>
>  Also, lines are numbered starting at 1 while the table is 0-indexed;
>>
> there seems
>
>> to be an off-by-1 error somewhere; or the comment needs to be
>>
> adjusted. An input
>
>> of 0 should not be accepted, either.
>>
>
> When I looked at this, the first entry in the table was always 0; in
> practice, the code below is (I believe) correct and does match the doc.
> I assumed that that had been done to avoid having to think about
> off-by-1 errors when manipulating the table, and I appreciated it at the
> time!
>
>
https://codereview.appspot.**com/12837044/<https://codereview.appspot.com/128...
>