Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(77)

Issue 142360043: code review 142360043: go/format, cmd/gofmt: fix issues with partial Go code w... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 11 months ago by shurcooL
Modified:
7 years, 10 months ago
Reviewers:
gri
CC:
golang-codereviews, bradfitz, gri
Visibility:
Public.

Description

go/format, cmd/gofmt: fix issues with partial Go code with indent Fixes issue 5551. Fixes issue 4449. Adds tests for both issues. Note that the two issues occur only when formatting partial Go code with indent. The best way to understand the change is as follows: I took the code of cmd/gofmt and go/format, combined it into one unified code that does not suffer from either 4449 nor 5551, and then applied that code to both cmd/gofmt and go/format. As a result, there is now much more identical code between the two packages, making future code deduplication easier (it was not possible to do that now without adding public APIs, which I was advised not to do at this time). More specifically, I took the parse() of cmd/gofmt which correctly preserves comments (issue 5551) and modified it to fix issue where it would sometimes modify literal values (issue 4449). I ended up removing the matchSpace() function because it no longer needed to do some of its work (insert indent), and a part of its work had to be done in advance (determining the indentation of first code line), because that calculation is required for cfg.Fprint() to run. adjustIndent is used to adjust the indent of cfg.Fprint() to compensate for the body of wrapper func being indented by one level. This allows to get rid of the bytes.Replace text manipulation of inner content, which was problematic and sometimes altered raw string literals (issue 4449). This means that sometimes the value of cfg.Indent is negative, but that works as expected. So now the algorithm for formatting partial Go code is: 1. Determine and prepend leading space of original source. 2. Determine and prepend indentation of first code line. 3. Format and write partial Go code (with all of its leading & trailing space trimmed). 4. Determine and append trailing space of original source.

Patch Set 1 #

Patch Set 2 : diff -r 932fe22207465e6c4bcdae29f5c519ba069f8927 https://code.google.com/p/go #

Patch Set 3 : diff -r 932fe22207465e6c4bcdae29f5c519ba069f8927 https://code.google.com/p/go #

Total comments: 18

Patch Set 4 : diff -r 48355ec55578b156b58c7df974acf132ab2e9d73 https://code.google.com/p/go #

Patch Set 5 : diff -r 48355ec55578b156b58c7df974acf132ab2e9d73 https://code.google.com/p/go #

Total comments: 6

Patch Set 6 : diff -r 1e2b51e32781fe475a580ffc8a7776507dfec1d5 https://code.google.com/p/go #

Patch Set 7 : diff -r 1e2b51e32781fe475a580ffc8a7776507dfec1d5 https://code.google.com/p/go #

Patch Set 8 : diff -r 032fdf145bf989b31b6c7d369c36c4b54066f55a https://code.google.com/p/go #

Patch Set 9 : diff -r 032fdf145bf989b31b6c7d369c36c4b54066f55a https://code.google.com/p/go #

Total comments: 30

Patch Set 10 : diff -r 8f36a32a9d036f86fb956c8538142eb384bd1a4a https://code.google.com/p/go #

Patch Set 11 : diff -r 8f36a32a9d036f86fb956c8538142eb384bd1a4a https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+315 lines, -165 lines) Patch
M src/cmd/gofmt/gofmt.go View 1 2 3 4 5 6 7 8 9 10 5 chunks +92 lines, -71 lines 0 comments Download
M src/cmd/gofmt/long_test.go View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
A src/cmd/gofmt/testdata/stdin6.golden View 1 2 3 4 5 1 chunk +19 lines, -0 lines 0 comments Download
A src/cmd/gofmt/testdata/stdin6.input View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
A src/cmd/gofmt/testdata/stdin7.golden View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
A src/cmd/gofmt/testdata/stdin7.input View 1 2 3 4 5 6 1 chunk +21 lines, -0 lines 0 comments Download
M src/go/format/format.go View 1 2 3 4 5 6 7 8 9 10 4 chunks +136 lines, -91 lines 0 comments Download
M src/go/format/format_test.go View 1 2 3 4 5 6 7 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 23
shurcooL
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
7 years, 11 months ago (2014-09-22 04:13:49 UTC) #1
bradfitz
R=gri On Mon, Sep 22, 2014 at 12:13 AM, <shurcooL@gmail.com> wrote: > Reviewers: golang-codereviews, > ...
7 years, 11 months ago (2014-09-22 13:15:51 UTC) #2
shurcooL
> As a result, there is now much more identical code between the two > ...
7 years, 11 months ago (2014-09-22 16:02:23 UTC) #3
gri
Some initial comments, more to come. This looks pretty good. Please don't yet factor out ...
7 years, 11 months ago (2014-09-22 18:53:52 UTC) #4
shurcooL
https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go File src/cmd/gofmt/gofmt.go (right): https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go#newcode124 src/cmd/gofmt/gofmt.go:124: for j < len(src) && isSpace(src[j]) { On 2014/09/22 ...
7 years, 11 months ago (2014-09-22 19:07:25 UTC) #5
gri
https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go File src/cmd/gofmt/gofmt.go (right): https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go#newcode124 src/cmd/gofmt/gofmt.go:124: for j < len(src) && isSpace(src[j]) { On 2014/09/22 ...
7 years, 11 months ago (2014-09-22 19:12:33 UTC) #6
shurcooL
> the two issues are very related By the way, I noticed earlier that issues ...
7 years, 11 months ago (2014-09-23 01:51:36 UTC) #7
shurcooL
Hello golang-codereviews@googlegroups.com, bradfitz@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
7 years, 11 months ago (2014-09-23 01:59:59 UTC) #8
gri
Some nitpicks, and some questions. Also, please add some more test cases per my comment ...
7 years, 11 months ago (2014-09-23 21:48:26 UTC) #9
shurcooL
https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go File src/cmd/gofmt/gofmt.go (right): https://codereview.appspot.com/142360043/diff/40001/src/cmd/gofmt/gofmt.go#newcode126 src/cmd/gofmt/gofmt.go:126: i = j + 1 // index of last ...
7 years, 11 months ago (2014-09-24 01:51:53 UTC) #10
shurcooL
On 2014/09/23 21:48:26, gri wrote: > Please add a couple of cases with raw strings ...
7 years, 11 months ago (2014-09-24 02:21:51 UTC) #11
gri
Sounds good. Thanks for the update. Regarding the 2*indent: I don't think the go/printed will ...
7 years, 11 months ago (2014-09-24 04:33:05 UTC) #12
shurcool_gmail.com
> Regarding the 2*indent: I don't think the go/printed will write indentation on empty lines. ...
7 years, 11 months ago (2014-09-24 04:43:19 UTC) #13
shurcool_gmail.com
> About the current issue with TrimSpace messing up raw string literals, I am still ...
7 years, 10 months ago (2014-09-24 19:05:27 UTC) #14
shurcooL
PTAL. Tests now pass, and I believe this should work, but it needs review. I've ...
7 years, 10 months ago (2014-09-25 04:35:05 UTC) #15
gri
Negative values do work, and "underflow" is just treated as 0. But I believe I ...
7 years, 10 months ago (2014-09-25 04:47:53 UTC) #16
gri
I think this looks good; and the negative indent adjustment is fine (also for go/printer). ...
7 years, 10 months ago (2014-09-25 17:36:06 UTC) #17
shurcooL
https://codereview.appspot.com/142360043/diff/160001/src/go/format/format.go File src/go/format/format.go (right): https://codereview.appspot.com/142360043/diff/160001/src/go/format/format.go#newcode90 src/go/format/format.go:90: var res []byte On 2014/09/25 17:36:05, gri wrote: > ...
7 years, 10 months ago (2014-09-26 06:03:35 UTC) #18
shurcooL
https://codereview.appspot.com/142360043/diff/160001/src/go/format/format.go File src/go/format/format.go (right): https://codereview.appspot.com/142360043/diff/160001/src/go/format/format.go#newcode181 src/go/format/format.go:181: // If the error is that the source file ...
7 years, 10 months ago (2014-09-26 06:54:22 UTC) #19
shurcooL
(Sorry for noise.) https://codereview.appspot.com/142360043/diff/160001/src/cmd/gofmt/gofmt.go File src/cmd/gofmt/gofmt.go (right): https://codereview.appspot.com/142360043/diff/160001/src/cmd/gofmt/gofmt.go#newcode109 src/cmd/gofmt/gofmt.go:109: var res []byte On 2014/09/25 17:36:05, ...
7 years, 10 months ago (2014-09-26 07:22:55 UTC) #20
shurcooL
PTAL. Ok, I've factored out the common sections into format() funcs. They are almost identical, ...
7 years, 10 months ago (2014-09-26 08:29:15 UTC) #21
gri
LGTM This is good enough for now. Thanks.
7 years, 10 months ago (2014-09-30 00:04:36 UTC) #22
gri
7 years, 10 months ago (2014-09-30 00:04:53 UTC) #23
*** Submitted as https://code.google.com/p/go/source/detail?r=70456f183225 ***

go/format, cmd/gofmt: fix issues with partial Go code with indent

Fixes issue 5551.
Fixes issue 4449.

Adds tests for both issues.

Note that the two issues occur only when formatting partial Go code
with indent.

The best way to understand the change is as follows: I took the code
of cmd/gofmt and go/format, combined it into one unified code that
does not suffer from either 4449 nor 5551, and then applied that code
to both cmd/gofmt and go/format.

As a result, there is now much more identical code between the two
packages, making future code deduplication easier (it was not possible
to do that now without adding public APIs, which I was advised not to
do at this time).

More specifically, I took the parse() of cmd/gofmt which correctly
preserves comments (issue 5551) and modified it to fix issue where
it would sometimes modify literal values (issue 4449).

I ended up removing the matchSpace() function because it no longer
needed to do some of its work (insert indent), and a part of its work
had to be done in advance (determining the indentation of first code
line), because that calculation is required for cfg.Fprint() to run.

adjustIndent is used to adjust the indent of cfg.Fprint() to compensate
for the body of wrapper func being indented by one level. This allows
to get rid of the bytes.Replace text manipulation of inner content,
which was problematic and sometimes altered raw string literals (issue
4449). This means that sometimes the value of cfg.Indent is negative,
but that works as expected.

So now the algorithm for formatting partial Go code is:

1. Determine and prepend leading space of original source.
2. Determine and prepend indentation of first code line.
3. Format and write partial Go code (with all of its leading &
   trailing space trimmed).
4. Determine and append trailing space of original source.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://codereview.appspot.com/142360043

Committer: Robert Griesemer <gri@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b