code review 111780045: strconv: fix CanBackquote for invalud UTF-8

strconv: fix CanBackquote for invalid UTF-8

Make CanBackquote(invalid UTF-8) return false.

Also add two test which show that CanBackquote reports
true for strings containing a BOM.

Fixes issue 7572.

[volker.dobler, 2014-07-02 12:49:33]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-07-02 13:50:16]

R=r

Btw, invalid spelling of invalid in subject of CL.




On Wed, Jul 2, 2014 at 5:49 AM, <dr.volker.dobler@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> strconv: fix CanBackquote for invalud UTF-8
>
> Make CanBackquote(invalid UTF-8) return false.
>
> Fixes issue 7572.
>
> Please review this at https://codereview.appspot.com/111780045/
>
> Affected files (+17, -3 lines):
>   M src/pkg/strconv/quote.go
>   M src/pkg/strconv/quote_test.go
>
>
> Index: src/pkg/strconv/quote.go
> ===================================================================
> --- a/src/pkg/strconv/quote.go
> +++ b/src/pkg/strconv/quote.go
> @@ -143,10 +143,22 @@
>  // unchanged as a single-line backquoted string without control
>  // characters other than space and tab.
>  func CanBackquote(s string) bool {
> -       for i := 0; i < len(s); i++ {
> +       i := 0
> +       for i < len(s) {
>                 c := s[i]
> -               if (c < ' ' && c != '\t') || c == '`' || c == '\u007F' {
> -                       return false
> +               if c < utf8.RuneSelf {
> +                       if (c < ' ' && c != '\t') || c == '`' || c ==
> '\u007F' {
> +                               return false
> +                       }
> +                       i++
> +               } else {
> +                       _, size := utf8.DecodeRuneInString(s[i:])
> +                       if size == 1 {
> +                               // A RuneError as all runes of size 1 have
> +                               // been handled above.
> +                               return false
> +                       }
> +                       i += size
>                 }
>         }
>         return true
> Index: src/pkg/strconv/quote_test.go
> ===================================================================
> --- a/src/pkg/strconv/quote_test.go
> +++ b/src/pkg/strconv/quote_test.go
> @@ -146,6 +146,8 @@
>         {`ABCDEFGHIJKLMNOPQRSTUVWXYZ`, true},
>         {`abcdefghijklmnopqrstuvwxyz`, true},
>         {`☺`, true},
> +       {"\x80", false},
> +       {"a\xe0\xa0z", false},
>  }
>
>  func TestCanBackquote(t *testing.T) {
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-07-07 19:49:29]

https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go
File src/pkg/strconv/quote.go (right):

https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go#...
src/pkg/strconv/quote.go:147: for i < len(s) {
we might need a benchmark to know if it's fast enough but i'd prefer this
version. it's more direct, although algorithmically equivalent.

for len(s) > 0 {
   r, wid := utf8.DecodeRuneInString(s)
   s = s[wid:]
   if wid > 1 {
       continue // All multibyte runes are correctly encoded and assumed
printable.
   }
   if r .... as below but we can avoid space ... return false
}

[r, 2014-07-07 19:50:21]

s/but we can ignore space// (i had a different snippet before)

On Mon, Jul 7, 2014 at 12:49 PM,  <r@golang.org> wrote:
>
> https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go
> File src/pkg/strconv/quote.go (right):
>
>
https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go#...
> src/pkg/strconv/quote.go:147: for i < len(s) {
> we might need a benchmark to know if it's fast enough but i'd prefer
> this version. it's more direct, although algorithmically equivalent.
>
> for len(s) > 0 {
>    r, wid := utf8.DecodeRuneInString(s)
>    s = s[wid:]
>    if wid > 1 {
>        continue // All multibyte runes are correctly encoded and assumed
> printable.
>    }
>    if r .... as below but we can avoid space ... return false
> }
>
> https://codereview.appspot.com/111780045/

[volker.dobler, 2014-07-08 09:11:38]

On 2014/07/07 19:49:29, r wrote:
> https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go
> File src/pkg/strconv/quote.go (right):
> 
>
https://codereview.appspot.com/111780045/diff/60001/src/pkg/strconv/quote.go#...
> src/pkg/strconv/quote.go:147: for i < len(s) {
> we might need a benchmark to know if it's fast enough but i'd prefer this
> version. it's more direct, although algorithmically equivalent.
> 
> for len(s) > 0 {
>    r, wid := utf8.DecodeRuneInString(s)
>    s = s[wid:]
>    if wid > 1 {
>        continue // All multibyte runes are correctly encoded and assumed
> printable.
>    }
>    if r .... as below but we can avoid space ... return false
> }

In Patch Set 5 (the latest, not for submission) I added benchmarks to
both versions. The ...Fast benchmarks use the original code, the others
your proposal:

BenchmarkCanBackquoteEasy	 5000000	269 ns/op
BenchmarkCanBackquoteHard	 5000000	385 ns/op
BenchmarkCanBackquoteEasyFast	30000000	 58.1 ns/op
BenchmarkCanBackquoteHardFast	 3000000	426 ns/op

For "easy" strings (i.e. 7-bit ASCII) the original version is
much faster.  For "hard" string containing only runes > utf8.RuneSelf
your version is a bit faster as the original but much more readable.

I cannot judge what type of string should be used in a realistic
benchmark.  Presumably CanBackquote is not very common in performance
critical inner loops: Your version ?

[r, 2014-07-10 18:04:59]

Since the prettier code isn't much slower, and is faster when things get messy,
let's go with that.

-rob

[volker.dobler, 2014-07-11 14:04:22]

PTAL

I think the behaviour on BOMs is wrong.
Leaving that for an other CL.

[r, 2014-07-15 02:46:26]

LGTM

[r, 2014-07-15 02:49:28]

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

strconv: fix CanBackquote for invalid UTF-8

Make CanBackquote(invalid UTF-8) return false.

Also add two test which show that CanBackquote reports
true for strings containing a BOM.

Fixes issue 7572.

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

Committer: Rob Pike <r@golang.org>

[gobot, 2014-07-15 02:59:31]

This CL appears to have broken the netbsd-386-minux builder.
See http://build.golang.org/log/6440ed96ba6f6e994de1adfd70ac14806de2c84e

[rsc, 2014-07-15 03:03:50]

FWIW, I agree that the behavior on BOMs is wrong.

[r, 2014-07-15 04:36:51]

And another CL is promised, no?

On Mon, Jul 14, 2014 at 8:03 PM,  <rsc@golang.org> wrote:
> FWIW, I agree that the behavior on BOMs is wrong.
>
>
> https://codereview.appspot.com/111780045/

[volker.dobler, 2014-07-15 04:49:30]

On Tue, Jul 15, 2014 at 6:36 AM, Rob Pike <r@golang.org> wrote:

> And another CL is promised, no?
>

Yes. In the next 6 hours.


> On Mon, Jul 14, 2014 at 8:03 PM,  <rsc@golang.org> wrote:
> > FWIW, I agree that the behavior on BOMs is wrong.
> >
> >
> > https://codereview.appspot.com/111780045/
>



-- 
Dr. Volker Dobler