code review 4634074: fmt: Added SkipSpace() function to fmt's ScanState inte...

fmt: Added SkipSpace() function to fmt's ScanState interface.

Users of the Scan() infrastructure that employ ReadRune() rather than
Token() need a way to skip leading spaces and newlines as set by the
the parent, Fscan(), Fscanln, or Fscanf(). As the internal methods and
boolean flags are not exported, this new function was added here and
in the Int and Nat Scan() functions of the big package. (fmt.Rat did
not need change since it uses Token()) Also added Printf style format
code support to int types and tests for same to int_test.go

[mtj, 2011-06-23 15:17:02]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-06-24 22:44:40]

LGTM for fmt changes.  leaving for gri to review the other piece.

[gri1, 2011-06-24 23:00:56]

Looks good, but the printing code needs test cases.

- gri

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362
src/pkg/big/int.go:362: 
please remove this extra line that crept in

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402
src/pkg/big/int.go:402: if x.neg {
use a switch here:

switch {
case x.neg:
    ...
case s.Flag('+'):
   ...
case s.Flag(' ') && ...
   ...
}

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472
src/pkg/big/int.go:472: 
I'd remove this empty line

[mtj1, 2011-06-24 23:03:33]

Great. The FMT piece is all I intended from you as I wanted to get your
blessing on the idea. The rest just implements all the widths, pad sides,
zero fillings, space insertions, etc. ("% x", "%-3d", "%+o", ...)

On Fri, Jun 24, 2011 at 3:44 PM, <r@golang.org> wrote:

> LGTM for fmt changes.  leaving for gri to review the other piece.
>
>
>
http://codereview.appspot.com/**4634074/<http://codereview.appspot.com/4634074/>
>



-- 

*Michael T. Jones*

   Chief Technology Advocate, Google Inc.

   1600 Amphitheatre Parkway, Mountain View, California
94043<http://maps.google.com/?ie=UTF8&ll=37.422313,-122.088048&spn=0.001135,0.001367&t=h&z=20>

   Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938

   *Organizing the world's information to make it universally accessible and
useful*

[mtj1, 2011-06-24 23:13:55]

I made test cases for all of these in int_test.go

On Fri, Jun 24, 2011 at 4:00 PM, <gri@google.com> wrote:

> Looks good, but the printing code needs test cases.
>
> - gri
>
>
>
>
http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**int.go<http:/...
> File src/pkg/big/int.go (right):
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode362<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362>
> src/pkg/big/int.go:362:
> please remove this extra line that crept in
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode402<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402>
> src/pkg/big/int.go:402: if x.neg {
> use a switch here:
>
> switch {
> case x.neg:
>    ...
> case s.Flag('+'):
>   ...
> case s.Flag(' ') && ...
>   ...
> }
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode472<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472>
> src/pkg/big/int.go:472:
> I'd remove this empty line
>
>
>
http://codereview.appspot.com/**4634074/<http://codereview.appspot.com/4634074/>
>



-- 

*Michael T. Jones*

   Chief Technology Advocate, Google Inc.

   1600 Amphitheatre Parkway, Mountain View, California
94043<http://maps.google.com/?ie=UTF8&ll=37.422313,-122.088048&spn=0.001135,0.001367&t=h&z=20>

   Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938

   *Organizing the world's information to make it universally accessible and
useful*

[mtj1, 2011-06-24 23:15:06]

to wit:

{"1234", "%d", "1234"},
{"1234", "%3d", "1234"},
 {"1234", "%4d", "1234"},
{"-1234", "%d", "-1234"},
 {"1234", "% 5d", " 1234"},
{"1234", "%+5d", "+1234"},
 {"1234", "%-5d", "1234 "},
{"1234", "%x", "4d2"},
 {"1234", "%X", "4D2"},
{"1234", "% x", "4 d2"},
 {"-1234", "%3x", "-4d2"},
{"-1234", "%4x", "-4d2"},
 {"-1234", "%5x", " -4d2"},
{"-1234", "%-5x", "-4d2 "},
 {"-1234", "% x", "-4 d2"},
{"1234", "%03d", "1234"},
 {"1234", "%04d", "1234"},
{"1234", "%05d", "01234"},
 {"1234", "%06d", "001234"},
{"-1234", "%06d", "-01234"},
 {"1234", "%+06d", "+01234"},
{"1234", "% 06d", " 01234"},
 {"1234", "%-6d", "1234  "},
{"1234", "%-06d", "001234"},
 {"-1234", "%-06d", "-01234"},

On Fri, Jun 24, 2011 at 4:13 PM, Michael Jones <mtj@google.com> wrote:

> I made test cases for all of these in int_test.go
>
> On Fri, Jun 24, 2011 at 4:00 PM, <gri@google.com> wrote:
>
>> Looks good, but the printing code needs test cases.
>>
>> - gri
>>
>>
>>
>>
http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**int.go<http:/...
>> File src/pkg/big/int.go (right):
>>
>> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>>
int.go#newcode362<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362>
>> src/pkg/big/int.go:362:
>> please remove this extra line that crept in
>>
>> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>>
int.go#newcode402<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402>
>> src/pkg/big/int.go:402: if x.neg {
>> use a switch here:
>>
>> switch {
>> case x.neg:
>>    ...
>> case s.Flag('+'):
>>   ...
>> case s.Flag(' ') && ...
>>   ...
>> }
>>
>> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>>
int.go#newcode472<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472>
>> src/pkg/big/int.go:472:
>> I'd remove this empty line
>>
>>
>>
http://codereview.appspot.com/**4634074/<http://codereview.appspot.com/4634074/>
>>
>
>
>
> --
>
> *Michael T. Jones*
>
>    Chief Technology Advocate, Google Inc.
>
>    1600 Amphitheatre Parkway, Mountain View, California
94043<http://maps.google.com/?ie=UTF8&ll=37.422313,-122.088048&spn=0.001135,0.001367&t=h&z=20>
>
>    Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938
>
>    *Organizing the world's information to make it universally accessible
> and useful*
>
>


-- 

*Michael T. Jones*

   Chief Technology Advocate, Google Inc.

   1600 Amphitheatre Parkway, Mountain View, California
94043<http://maps.google.com/?ie=UTF8&ll=37.422313,-122.088048&spn=0.001135,0.001367&t=h&z=20>

   Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938

   *Organizing the world's information to make it universally accessible and
useful*

[mtj1, 2011-06-24 23:17:17]

OK, will do, but the switch version will either depend on ordered evaluation
or repeat test clauses. The ordering seems part of the law of go, but still
feels like a risky lifestyle from years at c. ;-)

On Fri, Jun 24, 2011 at 4:00 PM, <gri@google.com> wrote:

> Looks good, but the printing code needs test cases.
>
> - gri
>
>
>
>
http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**int.go<http:/...
> File src/pkg/big/int.go (right):
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode362<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362>
> src/pkg/big/int.go:362:
> please remove this extra line that crept in
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode402<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402>
> src/pkg/big/int.go:402: if x.neg {
> use a switch here:
>
> switch {
> case x.neg:
>    ...
> case s.Flag('+'):
>   ...
> case s.Flag(' ') && ...
>   ...
> }
>
> http://codereview.appspot.com/**4634074/diff/6001/src/pkg/big/**
>
int.go#newcode472<http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472>
> src/pkg/big/int.go:472:
> I'd remove this empty line
>
>
>
http://codereview.appspot.com/**4634074/<http://codereview.appspot.com/4634074/>
>



-- 

*Michael T. Jones*

   Chief Technology Advocate, Google Inc.

   1600 Amphitheatre Parkway, Mountain View, California
94043<http://maps.google.com/?ie=UTF8&ll=37.422313,-122.088048&spn=0.001135,0.001367&t=h&z=20>

   Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938

   *Organizing the world's information to make it universally accessible and
useful*

[r2, 2011-06-24 23:18:24]

the ordering is the law, and it's wonderful.

-rob

[gri1, 2011-06-24 23:21:45]

Please add int_test.go to this CL (hg change <CL number>, add the
file), and them "hg mail <CL number>" (this will send me an automated
PTAL e-mail). Thanks.
- Robert

On Fri, Jun 24, 2011 at 4:13 PM, Michael Jones <mtj@google.com> wrote:
> I made test cases for all of these in int_test.go
> On Fri, Jun 24, 2011 at 4:00 PM, <gri@google.com> wrote:
>>
>> Looks good, but the printing code needs test cases.
>>
>> - gri
>>
>>
>>
>> http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go
>> File src/pkg/big/int.go (right):
>>
>>
>> http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362
>> src/pkg/big/int.go:362:
>> please remove this extra line that crept in
>>
>>
>> http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402
>> src/pkg/big/int.go:402: if x.neg {
>> use a switch here:
>>
>> switch {
>> case x.neg:
>>    ...
>> case s.Flag('+'):
>>   ...
>> case s.Flag(' ') && ...
>>   ...
>> }
>>
>>
>> http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472
>> src/pkg/big/int.go:472:
>> I'd remove this empty line
>>
>> http://codereview.appspot.com/4634074/
>
>
>
> --
>
> Michael T. Jones
>
>    Chief Technology Advocate, Google Inc.
>
>    1600 Amphitheatre Parkway, Mountain View, California 94043
>
>    Email: mtj@google.com  Mobile: 650-335-5765  Fax: 650-649-1938
>
>    Organizing the world's information to make it universally accessible and
> useful
>

[mtj1, 2011-06-24 23:28:15]

all done as suggested, builds and tests without error

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode362
src/pkg/big/int.go:362: 
On 2011/06/24 23:00:56, gri1 wrote:
> please remove this extra line that crept in

Done.

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode402
src/pkg/big/int.go:402: if x.neg {
On 2011/06/24 23:00:56, gri1 wrote:
> use a switch here:
> 
> switch {
> case x.neg:
>     ...
> case s.Flag('+'):
>    ...
> case s.Flag(' ') && ...
>    ...
> }

Done.

http://codereview.appspot.com/4634074/diff/6001/src/pkg/big/int.go#newcode472
src/pkg/big/int.go:472: 
On 2011/06/24 23:00:56, gri1 wrote:
> I'd remove this empty line

Done.

[r2, 2011-06-24 23:40:45]

hg clpatch 4634074
edit ....

[mtj, 2011-06-25 00:10:12]

Hello r@google.com, r@golang.org, gri@google.com, mtj@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mtj, 2011-06-25 00:13:10]

Hello r@google.com, r@golang.org, gri@google.com, mtj@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mtj, 2011-06-25 00:19:35]

Hello r@google.com, r@golang.org, gri@google.com, mtj@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri1, 2011-06-25 00:25:09]

LGTM
thanks!
- gri


On Fri, Jun 24, 2011 at 5:19 PM,  <michael.jones@gmail.com> wrote:
> Hello r@google.com, r@golang.org, gri@google.com, mtj@google.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4634074/
>

[gri, 2011-06-25 00:26:48]

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

fmt: Added SkipSpace() function to fmt's ScanState interface.

Users of the Scan() infrastructure that employ ReadRune() rather than
Token() need a way to skip leading spaces and newlines as set by the
the parent, Fscan(), Fscanln, or Fscanf(). As the internal methods and
boolean flags are not exported, this new function was added here and
in the Int and Nat Scan() functions of the big package. (fmt.Rat did
not need change since it uses Token()) Also added Printf style format
code support to int types and tests for same to int_test.go

R=r, r, gri, mtj
CC=golang-dev
http://codereview.appspot.com/4634074

Committer: Robert Griesemer <gri@golang.org>