code review 4561061: strconv: change Quote to be Unicode-friendly,

strconv: change Quote to be Unicode-friendly,
add QuoteToASCII.
The Quote and QuoteRune functions now let printable
runes (as defined by unicode.IsPrint) through.  When
true 7-bit clean stuff is necessary, there are now two
new functions: QuoteToASCII and QuoteRuneToASCII.

Printf("%q") uses Quote. To get the old behavior, it
will now be necessary to say
        Printf("%s", strconv.QuoteToASCII(s))
but that should rarely be necessary.

[r, 2011-06-03 23:17:35]

Hello golang-dev@googlegroups.com,

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

[r, 2011-06-03 23:27:01]

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

Please take another look.

[gri, 2011-06-03 23:45:28]

LGMT

http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode17
src/pkg/strconv/quote.go:17: func quoteWith(s string, quote byte, ASCIIonly
bool) string {
s/byte/int/

and you don't need the conversions

http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode34
src/pkg/strconv/quote.go:34: if ASCIIonly {
s/</<=/ ? (to be consistent with the previous CL?)

http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode61
src/pkg/strconv/quote.go:61: caseEscX:
Can we call this perhaps "handleEscX" ? it almost looks like a typo/case label
since it also starts with case. I did a double take. (cute, though)

http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode71
src/pkg/strconv/quote.go:71: rune = 0xFFFD
shouldn't  this be written with \u instead of \U ? (0xFFFD < 0x10000)

[r, 2011-06-03 23:55:14]

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

Please take another look.

[r2, 2011-06-03 23:55:52]

On 04/06/2011, at 9:45 AM, gri@golang.org wrote:

> LGMT
> 
> 
> http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go
> File src/pkg/strconv/quote.go (right):
> 
>
http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode17
> src/pkg/strconv/quote.go:17: func quoteWith(s string, quote byte,
> ASCIIonly bool) string {
> s/byte/int/
> 
> and you don't need the conversions

then i'd need them in WriteByte.

>
http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode34
> src/pkg/strconv/quote.go:34: if ASCIIonly {
> s/</<=/ ? (to be consistent with the previous CL?)

yes.

>
http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode61
> src/pkg/strconv/quote.go:61: caseEscX:
> Can we call this perhaps "handleEscX" ? it almost looks like a typo/case
> label since it also starts with case. I did a double take. (cute,
> though)

done.

>
http://codereview.appspot.com/4561061/diff/5/src/pkg/strconv/quote.go#newcode71
> src/pkg/strconv/quote.go:71: rune = 0xFFFD
> shouldn't  this be written with \u instead of \U ? (0xFFFD < 0x10000)

good catch.

-rob

[gri, 2011-06-04 00:08:41]

LGTM

http://codereview.appspot.com/4561061/diff/5001/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

http://codereview.appspot.com/4561061/diff/5001/src/pkg/strconv/quote.go#newc...
src/pkg/strconv/quote.go:70: for j := uint(0); j < 4; j++ {
for s := 12; s >= 0; s -= 4 {
   ... rune>>s&0xF...

seems simpler

same below

[gri, 2011-06-04 00:09:14]

of course: s := uint(12 ...

On Fri, Jun 3, 2011 at 5:08 PM,  <gri@golang.org> wrote:
> LGTM
>
>
> http://codereview.appspot.com/4561061/diff/5001/src/pkg/strconv/quote.go
> File src/pkg/strconv/quote.go (right):
>
>
http://codereview.appspot.com/4561061/diff/5001/src/pkg/strconv/quote.go#newc...
> src/pkg/strconv/quote.go:70: for j := uint(0); j < 4; j++ {
> for s := 12; s >= 0; s -= 4 {
>   ... rune>>s&0xF...
>
> seems simpler
>
> same below
>
> http://codereview.appspot.com/4561061/
>

[r, 2011-06-04 00:18:18]

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

Please take another look.

[gri, 2011-06-04 00:27:03]

LGTM

http://codereview.appspot.com/4561061/diff/10001/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

http://codereview.appspot.com/4561061/diff/10001/src/pkg/strconv/quote.go#new...
src/pkg/strconv/quote.go:71: buf.WriteByte(lowerhex[(rune>>s)&0xF])
no need for the ()'s around the shift

http://codereview.appspot.com/4561061/diff/10001/src/pkg/strconv/quote.go#new...
src/pkg/strconv/quote.go:76: buf.WriteByte(lowerhex[(rune>>(s))&0xF])
s/(s)/s/

Also, you don't need the () around the shift, >> and & are both multiplicative
and thus at the same precedence level and the expression is evaluated from left
to right. It's not C (but it's fine to keep them if you feel it increases
readability).

[gri, 2011-06-04 00:28:33]

PS: perhaps there should be a test case for 0xFFFD .

On Fri, Jun 3, 2011 at 5:18 PM,  <r@golang.org> wrote:
> Hello golang-dev@googlegroups.com, gri@golang.org, r@google.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4561061/
>

[r, 2011-06-04 06:40:02]

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

Please take another look.

[r2, 2011-06-04 06:40:41]

your rewrite needed work because uints can't go negative, but it was easy to
fix.

waiting for rsc, since this is his code i'm hacking.

-rob

[r, 2011-06-07 12:23:15]

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

strconv: change Quote to be Unicode-friendly,
add QuoteToASCII.
The Quote and QuoteRune functions now let printable
runes (as defined by unicode.IsPrint) through.  When
true 7-bit clean stuff is necessary, there are now two
new functions: QuoteToASCII and QuoteRuneToASCII.

Printf("%q") uses Quote. To get the old behavior, it
will now be necessary to say
        Printf("%s", strconv.QuoteToASCII(s))
but that should rarely be necessary.

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

[rsc, 2011-06-07 19:56:35]

LGTM