code review 5234041: utf8: add Valid and ValidString

utf8: add Valid and ValidString

[bradfitz, 2011-10-07 01:43:24]

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

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

[rsc, 2011-10-07 01:50:48]

The correct test is r == RuneError && size == 1.
"a\uFFFDb" is a valid UTF-8 string (and it should be
a test case).

[bradfitz, 2011-10-07 01:55:00]

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

Please take another look.

[bradfitz, 2011-10-07 01:55:07]

Done & done.

On Thu, Oct 6, 2011 at 6:50 PM, Russ Cox <rsc@golang.org> wrote:

> The correct test is r == RuneError && size == 1.
> "a\uFFFDb" is a valid UTF-8 string (and it should be
> a test case).
>

[rsc, 2011-10-07 01:59:42]

The code looks fine.  I will leave the API question for Rob.

What is this for?

Russ

[bradfitz, 2011-10-07 02:01:33]

I asked Rob if this existed already.  He said no, but it should, and he
suggested the names.

I had a buggy version of this copy/pasted around in various spots.


On Thu, Oct 6, 2011 at 6:59 PM, Russ Cox <rsc@golang.org> wrote:

> The code looks fine.  I will leave the API question for Rob.
>
> What is this for?
>
> Russ
>
>

[brainman, 2011-10-07 02:43:08]

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8_test.go
File src/pkg/utf8/utf8_test.go (right):

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8_test.go#new...
src/pkg/utf8/utf8_test.go:287: {"ЖбрэдЖ", true},
That is not your name, "Брэд" is.

[r, 2011-10-07 03:24:10]

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go
File src/pkg/utf8/utf8.go (right):

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode358
src/pkg/utf8/utf8.go:358: // Valid reports whether p consists entirely of valid
UTF-8 encoded runes.
here and below,
s/UTF-8 encoded/UTF-8-encoded/  (adjective phrases are hyphenated)

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode366
src/pkg/utf8/utf8.go:366: if r == RuneError && size == 1 {
i don't believe you need the RuneError check. you just verified that the rune
isn't an ascii byte, so if it's got a width of 1 now, that's an error.
not sure it matters but why check both?
tricky enough to deserve a comment.

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode380
src/pkg/utf8/utf8.go:380: if r == RuneError && size == 1 {
ditto

[bradfitz, 2011-10-07 03:41:22]

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go
File src/pkg/utf8/utf8.go (right):

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode358
src/pkg/utf8/utf8.go:358: // Valid reports whether p consists entirely of valid
UTF-8 encoded runes.
On 2011/10/07 03:24:10, r wrote:
> here and below,
> s/UTF-8 encoded/UTF-8-encoded/  (adjective phrases are hyphenated)

Done.

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode366
src/pkg/utf8/utf8.go:366: if r == RuneError && size == 1 {
On 2011/10/07 03:24:10, r wrote:
> i don't believe you need the RuneError check. you just verified that the rune
> isn't an ascii byte, so if it's got a width of 1 now, that's an error.
> not sure it matters but why check both?

sure, done.

> tricky enough to deserve a comment.

done.

http://codereview.appspot.com/5234041/diff/7001/src/pkg/utf8/utf8.go#newcode380
src/pkg/utf8/utf8.go:380: if r == RuneError && size == 1 {
On 2011/10/07 03:24:10, r wrote:
> ditto

Done.

[bradfitz, 2011-10-07 03:41:24]

Hello r@golang.org, rsc@golang.org, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-10-07 04:30:33]

LGTM modulo Russian

http://codereview.appspot.com/5234041/diff/6002/src/pkg/utf8/utf8_test.go
File src/pkg/utf8/utf8_test.go (right):

http://codereview.appspot.com/5234041/diff/6002/src/pkg/utf8/utf8_test.go#new...
src/pkg/utf8/utf8_test.go:287: {"ЖбрэдЖ", true},
did you want to use alex's spelling?

[bradfitz, 2011-10-07 05:47:30]

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

utf8: add Valid and ValidString

R=r, rsc, alex.brainman
CC=golang-dev
http://codereview.appspot.com/5234041

[brainman, 2011-10-07 05:50:11]

ЛГТМ