code review 43610043: net: reimplement toLower to not depend on strings

net: reimplement toLower to not depend on strings

[0intro, 2013-12-17 21:47:05]

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

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

[r, 2013-12-17 21:49:05]

https://codereview.appspot.com/43610043/diff/30001/src/pkg/net/lookup_plan9.go
File src/pkg/net/lookup_plan9.go (right):

https://codereview.appspot.com/43610043/diff/30001/src/pkg/net/lookup_plan9.g...
src/pkg/net/lookup_plan9.go:72: func toLower(in string) string {
needs a comment to say why it's not using the strings package and why ascii is
sufficient.

[bradfitz, 2013-12-17 21:49:45]

Also, I would avoid the allocation and just reuse 'in' if it's already all
lowercase.



On Tue, Dec 17, 2013 at 1:47 PM, <0intro@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: reimplement toLower to not depend on strings
>
> Please review this at https://codereview.appspot.com/43610043/
>
> Affected files (+11, -2 lines):
>   M src/pkg/net/lookup_plan9.go
>
>
> Index: src/pkg/net/lookup_plan9.go
> ===================================================================
> --- a/src/pkg/net/lookup_plan9.go
> +++ b/src/pkg/net/lookup_plan9.go
> @@ -7,7 +7,6 @@
>  import (
>         "errors"
>         "os"
> -       "strings"
>  )
>
>  func query(filename, query string, bufSize int) (res []string, err error)
> {
> @@ -70,10 +69,20 @@
>         return query("/net/dns", addr+" "+typ, 1024)
>  }
>
> +func toLower(in string) string {
> +       out := []byte(in)
> +       for i, c := range out {
> +               if 'A' <= c && c <= 'Z' {
> +                       out[i] += 'a' - 'A'
> +               }
> +       }
> +       return string(out)
> +}
> +
>  // lookupProtocol looks up IP protocol name and returns
>  // the corresponding protocol number.
>  func lookupProtocol(name string) (proto int, err error) {
> -       lines, err := query("/net/cs", "!protocol="+strings.ToLower(name),
> 128)
> +       lines, err := query("/net/cs", "!protocol="+toLower(name), 128)
>         if err != nil {
>                 return 0, err
>         }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[bradfitz, 2013-12-17 21:49:58]

allocations, plural



On Tue, Dec 17, 2013 at 1:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Also, I would avoid the allocation and just reuse 'in' if it's already all
> lowercase.
>
>
>
> On Tue, Dec 17, 2013 at 1:47 PM, <0intro@gmail.com> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> net: reimplement toLower to not depend on strings
>>
>> Please review this at https://codereview.appspot.com/43610043/
>>
>> Affected files (+11, -2 lines):
>>   M src/pkg/net/lookup_plan9.go
>>
>>
>> Index: src/pkg/net/lookup_plan9.go
>> ===================================================================
>> --- a/src/pkg/net/lookup_plan9.go
>> +++ b/src/pkg/net/lookup_plan9.go
>> @@ -7,7 +7,6 @@
>>  import (
>>         "errors"
>>         "os"
>> -       "strings"
>>  )
>>
>>  func query(filename, query string, bufSize int) (res []string, err
>> error) {
>> @@ -70,10 +69,20 @@
>>         return query("/net/dns", addr+" "+typ, 1024)
>>  }
>>
>> +func toLower(in string) string {
>> +       out := []byte(in)
>> +       for i, c := range out {
>> +               if 'A' <= c && c <= 'Z' {
>> +                       out[i] += 'a' - 'A'
>> +               }
>> +       }
>> +       return string(out)
>> +}
>> +
>>  // lookupProtocol looks up IP protocol name and returns
>>  // the corresponding protocol number.
>>  func lookupProtocol(name string) (proto int, err error) {
>> -       lines, err := query("/net/cs", "!protocol="+strings.ToLower(name),
>> 128)
>> +       lines, err := query("/net/cs", "!protocol="+toLower(name), 128)
>>         if err != nil {
>>                 return 0, err
>>         }
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>
>

[0intro, 2013-12-17 22:04:52]

https://codereview.appspot.com/43610043/diff/30001/src/pkg/net/lookup_plan9.go
File src/pkg/net/lookup_plan9.go (right):

https://codereview.appspot.com/43610043/diff/30001/src/pkg/net/lookup_plan9.g...
src/pkg/net/lookup_plan9.go:72: func toLower(in string) string {
On 2013/12/17 21:49:05, r wrote:
> needs a comment to say why it's not using the strings package and why ascii is
> sufficient.

Done.

[bradfitz, 2013-12-17 22:18:27]

LGTM

[bradfitz, 2013-12-17 22:19:14]

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

net: reimplement toLower to not depend on strings

R=golang-dev, r, bradfitz
CC=golang-dev, jas
https://codereview.appspot.com/43610043

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[r, 2013-12-17 22:21:35]

https://codereview.appspot.com/43610043/diff/50001/src/pkg/net/lookup_plan9.go
File src/pkg/net/lookup_plan9.go (right):

https://codereview.appspot.com/43610043/diff/50001/src/pkg/net/lookup_plan9.g...
src/pkg/net/lookup_plan9.go:93: }
this is fine but it could involve less jumping around. also your second loop is
wrong if there's non-ascii, which there isn't but why get it wrong anyway?

func toLower(in string) string {
    for _, c := range in {
        if 'A' <= c && c <= 'Z' {
            // Has upper case; need to fix.
            out := []byte(in)
            for i := 0; i < len(in); i++ {
                c := in[i]
                if 'A' <= c && c <= 'Z' {
                    c += 'a' -'A'
                }
                out[i] = c
            }
            return string(out)
        }
    }
    return in
}

[0intro, 2013-12-17 22:36:10]

Thanks. Your version does indeed look better.
It seems Brad already submitted the change however. Could I create a new CL with
your suggested change?

[r, 2013-12-17 22:51:42]

sure, go for it. thanks.

-rob