code review 76730043: strconv.CanBackquote: Handle DEL as control character.

strconv: fix CanBackquote for DEL

CanBackquote is expected to return false if a given string contains
control characters other than space and tab. DEL is a control charcater
so CanBackquote should return false for it.

[ruiu, 2014-03-17 04:08:20]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-03-17 19:11:19]

Subject of the patch should be just "strconv: xxxx" where xxx starts
lowercase.  Also don't need need trailing period.

"strconv: fix CanBackquote for DEL"

But this function seems wrong for other reasons, for invalid UTF-8:
http://play.golang.org/p/AzDqsSnT4f



On Sun, Mar 16, 2014 at 9:08 PM, <ruiu@google.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.CanBackquote: Handle DEL as control character.
>
> CanBackquote is expected to return false if a given string contains
> control characters other than space and tab. DEL is a control charcater
> so CanBackquote should return false for it.
>
> Please review this at https://codereview.appspot.com/76730043/
>
> Affected files (+2, -1 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
> @@ -144,7 +144,7 @@
>  // characters other than space and tab.
>  func CanBackquote(s string) bool {
>         for i := 0; i < len(s); i++ {
> -               if (s[i] < ' ' && s[i] != '\t') || s[i] == '`' {
> +               if (s[i] < ' ' && s[i] != '\t') || s[i] == '`' || s[i] ==
> '\x7F' {
>                         return false
>                 }
>         }
> Index: src/pkg/strconv/quote_test.go
> ===================================================================
> --- a/src/pkg/strconv/quote_test.go
> +++ b/src/pkg/strconv/quote_test.go
> @@ -140,6 +140,7 @@
>         {string(29), false},
>         {string(30), false},
>         {string(31), false},
> +       {string(127), false}, // DEL
>         {`' !"#$%&'()*+,-./:;<=>?@[\]^_{|}~`, true},
>         {`0123456789`, true},
>         {`ABCDEFGHIJKLMNOPQRSTUVWXYZ`, true},
>
>
> --
> 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.
>

[bradfitz, 2014-03-17 19:15:06]

Filed https://code.google.com/p/go/issues/detail?id=7565



On Mon, Mar 17, 2014 at 12:11 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Subject of the patch should be just "strconv: xxxx" where xxx starts
> lowercase.  Also don't need need trailing period.
>
> "strconv: fix CanBackquote for DEL"
>
> But this function seems wrong for other reasons, for invalid UTF-8:
> http://play.golang.org/p/AzDqsSnT4f
>
>
>
> On Sun, Mar 16, 2014 at 9:08 PM, <ruiu@google.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.CanBackquote: Handle DEL as control character.
>>
>> CanBackquote is expected to return false if a given string contains
>> control characters other than space and tab. DEL is a control charcater
>> so CanBackquote should return false for it.
>>
>> Please review this at https://codereview.appspot.com/76730043/
>>
>> Affected files (+2, -1 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
>> @@ -144,7 +144,7 @@
>>  // characters other than space and tab.
>>  func CanBackquote(s string) bool {
>>         for i := 0; i < len(s); i++ {
>> -               if (s[i] < ' ' && s[i] != '\t') || s[i] == '`' {
>> +               if (s[i] < ' ' && s[i] != '\t') || s[i] == '`' || s[i] ==
>> '\x7F' {
>>                         return false
>>                 }
>>         }
>> Index: src/pkg/strconv/quote_test.go
>> ===================================================================
>> --- a/src/pkg/strconv/quote_test.go
>> +++ b/src/pkg/strconv/quote_test.go
>> @@ -140,6 +140,7 @@
>>         {string(29), false},
>>         {string(30), false},
>>         {string(31), false},
>> +       {string(127), false}, // DEL
>>         {`' !"#$%&'()*+,-./:;<=>?@[\]^_{|}~`, true},
>>         {`0123456789`, true},
>>         {`ABCDEFGHIJKLMNOPQRSTUVWXYZ`, true},
>>
>>
>> --
>> 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.
>>
>
>