code review 5180044: time: make month/day name comparisons case insenstive

time: make month/day name comparisons case insenstive

Fixes issue 2324.

[borman, 2011-10-03 21:54:13]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2011-10-03 21:55:18]

Should have a test, otherwise you didn't really fix anything (at least
long-term).

On Mon, Oct 3, 2011 at 2:54 PM, <borman@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> pkg/time: make month/day name comparisons case insenstive
>
> Addresses issue 2324 "pkg/time: time.Parse is case sensitive"
>
> Please review this at
http://codereview.appspot.com/**5180044/<http://codereview.appspot.com/5180044/>
>
> Affected files:
>  M src/pkg/time/format.go
>
>
> Index: src/pkg/time/format.go
> ==============================**==============================**=======
> --- a/src/pkg/time/format.go
> +++ b/src/pkg/time/format.go
> @@ -4,6 +4,7 @@
>        "bytes"
>        "os"
>        "strconv"
> +       "strings"
>  )
>
>  const (
> @@ -234,7 +235,7 @@
>
>  func lookup(tab []string, val string) (int, string, os.Error) {
>        for i, v := range tab {
> -               if len(val) >= len(v) && val[0:len(v)] == v {
> +               if len(val) >= len(v) && strings.EqualFold(val[0:len(v)**],
> v) {
>                        return i, val[len(v):], nil
>                }
>        }
>
>
>

[dsymonds, 2011-10-03 21:58:43]

Add a test.

Also, the CL description should just start with "time:", not "pkg/time:".

[rsc, 2011-10-03 22:09:50]

R=r

[borman, 2011-10-03 22:10:35]

On 2011/10/03 21:58:43, dsymonds wrote:
> Add a test.
> 
> Also, the CL description should just start with "time:", not "pkg/time:".

Done and Done.

[r, 2011-10-03 22:29:51]

http://codereview.appspot.com/5180044/diff/7001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/7001/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:238: if len(val) >= len(v) &&
strings.EqualFold(val[0:len(v)], v) {
this is overkill and dependency-injecting. ASCII is good enough - the layout
strings are ASCII. we can revisit this when we have proper internationalization.

[borman, 2011-10-04 16:22:25]

PTAL

I removed the dependency on the strings package.  I didn't see much option but
to write my own version of strcasecmp, however.

[r, 2011-10-04 17:30:06]

http://codereview.appspot.com/5180044/diff/14001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/14001/src/pkg/time/format.go#newco...
src/pkg/time/format.go:235: func strcaseeq(s1, s2 string) bool {
this isn't C. "equal" would be fine, with an appropriate comment.

[r, 2011-10-04 17:30:28]

or if you don't like "equal", go with "match".

[borman, 2011-10-04 17:34:34]

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

Please take another look.

[r, 2011-10-04 17:40:44]

http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
src/pkg/time/format.go:238: for i, c1 := range s1 {
the strings are ascii and the rest of the code assumes they are. might as well
loop on bytes and avoid the conversions.

http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
src/pkg/time/format.go:242: if c1 < 'a' || c1 > 'z' || (c2|' ') != c1 {
i'm a fan of or'ing with ' ' but i've been yelled at too often for being
obscure. today's programmers don't know what a bit is, let alone have ascii
memorized, so just write the addition.

sorry. sniff.

[borman, 2011-10-04 17:45:00]

On 2011/10/04 17:40:44, r wrote:
> http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go
> File src/pkg/time/format.go (right):
> 
>
http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
> src/pkg/time/format.go:238: for i, c1 := range s1 {
> the strings are ascii and the rest of the code assumes they are. might as well
> loop on bytes and avoid the conversions.
> 
>
http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
> src/pkg/time/format.go:242: if c1 < 'a' || c1 > 'z' || (c2|' ') != c1 {
> i'm a fan of or'ing with ' ' but i've been yelled at too often for being
> obscure. today's programmers don't know what a bit is, let alone have ascii
> memorized, so just write the addition.
> 
> sorry. sniff.

Well, I guess it is confusing to EBCDIC users...

[borman, 2011-10-04 17:46:55]

On 2011/10/04 17:45:00, borman wrote:
> On 2011/10/04 17:40:44, r wrote:
> > http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go
> > File src/pkg/time/format.go (right):
> > 
> >
>
http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
> > src/pkg/time/format.go:238: for i, c1 := range s1 {
> > the strings are ascii and the rest of the code assumes they are. might as
well
> > loop on bytes and avoid the conversions.
> > 
> >
>
http://codereview.appspot.com/5180044/diff/10002/src/pkg/time/format.go#newco...
> > src/pkg/time/format.go:242: if c1 < 'a' || c1 > 'z' || (c2|' ') != c1 {
> > i'm a fan of or'ing with ' ' but i've been yelled at too often for being
> > obscure. today's programmers don't know what a bit is, let alone have ascii
> > memorized, so just write the addition.
> > 
> > sorry. sniff.
> 
> Well, I guess it is confusing to EBCDIC users...

I don't think you are going to like the "fix".  You can't use addition, but I
did get rid of the ' '.

[borman, 2011-10-04 17:47:11]

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

Please take another look.

[r, 2011-10-04 17:59:46]

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:238: for i, c1 := range s1 {
this is still a range; use a loop and do bytes, as i said previously, and the
drop the conversions

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:241: c1 |= 'a' - 'A'
c2 |= 'a' - 'A'

also // 'a'-'A' is known to be a single bit.

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:242: if c1 < 'a' || c1 > 'z' || c2|('a'-'A') != c1 {
now drop the | here - it's cleaner to do it before.

[borman, 2011-10-04 18:14:53]

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:238: for i, c1 := range s1 {
On 2011/10/04 17:59:46, r wrote:
> this is still a range; use a loop and do bytes, as i said previously, and the
> drop the conversions

Okay, sorry.  I forget that range on string does not always return indicies 0,
1, 2, ...  Was using range to eliminate the len() per byte.  I could do it in
reverse to ge the same but I will go with the straight forward code.

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:241: c1 |= 'a' - 'A'
On 2011/10/04 17:59:46, r wrote:
> c2 |= 'a' - 'A'
> 
> also // 'a'-'A' is known to be a single bit.

Done.

http://codereview.appspot.com/5180044/diff/10005/src/pkg/time/format.go#newco...
src/pkg/time/format.go:242: if c1 < 'a' || c1 > 'z' || c2|('a'-'A') != c1 {
On 2011/10/04 17:59:46, r wrote:
> now drop the | here - it's cleaner to do it before.

Done.

[borman, 2011-10-04 18:15:10]

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

Please take another look.

[r, 2011-10-04 18:19:23]

http://codereview.appspot.com/5180044/diff/19001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5180044/diff/19001/src/pkg/time/format.go#newco...
src/pkg/time/format.go:242: // 'a' - 'A' is know to be a single bit that
s/know/known/
s/ that/./
or maybe
// Move to lower-case; 'a'-'A' is known to be a single bit.

[borman, 2011-10-04 18:21:22]

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

Please take another look.

[r, 2011-10-04 18:23:12]

LGTM

[r2, 2011-10-04 18:23:49]

one last thing.
please change the CL to read

Fixes issue 2324.

in exactly that format.

On Oct 4, 2011, at 11:23 AM, r@golang.org wrote:

> LGTM
> 
> 
> http://codereview.appspot.com/5180044/

[r, 2011-10-04 19:52:35]

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

time: make month/day name comparisons case insenstive

Fixes issue 2324.

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

Committer: Rob Pike <r@golang.org>