code review 4172049: gc: make string const comparison unsigned

gc: make string const comparison unsigned

Make compile-time string const comparison match semantics
of runtime.cmpstring.

Fixes issue 1515.

[jeff.allen, 2011-02-15 12:20:37]

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

I'd like you to review this change.

[rsc1, 2011-02-15 13:48:56]

http://codereview.appspot.com/4172049/diff/2001/src/cmd/gc/const.c
File src/cmd/gc/const.c (right):

http://codereview.appspot.com/4172049/diff/2001/src/cmd/gc/const.c#newcode1058
src/cmd/gc/const.c:1058: s1 = l->val.u.sval->s;
Doesn't this now need a cast?
Same on next line.

[jeff.allen, 2011-02-15 16:28:29]

http://codereview.appspot.com/4172049/diff/2001/src/cmd/gc/const.c
File src/cmd/gc/const.c (right):

http://codereview.appspot.com/4172049/diff/2001/src/cmd/gc/const.c#newcode1058
src/cmd/gc/const.c:1058: s1 = l->val.u.sval->s;
The stype of sval->s is char *. And now that you mention it, this change does
cause a warning. Oops.

Is a cast the right answer, or fixing struct Strlit to have uchar s[3]? I don't
have enough familiarity to understand if changing Strlit is too big of a change.
A quick experiment turned up 22 warnings as a result of changing Strlit, thus
~20 other locations to fix, thus an uglier CL.

Tell me which way to go and I'll send in another CL.

Have a nice day.
 -jeff

[rsc, 2011-02-15 16:35:06]

Please just add the cast in the one place.

[jeff.allen, 2011-02-16 11:24:57]

*** Abandoned ***

[jeff.allen, 2011-02-16 12:01:15]

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

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

[rsc, 2011-02-16 16:29:24]

On Wed, Feb 16, 2011 at 06:24,  <jeff.allen@gmail.com> wrote:
> *** Abandoned ***

If you are trying to delete the CL from one machine
you should use hg change -D instead of hg change -d.
Using the latter sends this mail and also marks the issue
as closed on the codereview server, so that it no longer
appears in our dashboards.

[rsc, 2011-02-16 18:20:15]

almost there... 
always a good idea to look at the surrounding code
to see how things are idiomatically written.

http://codereview.appspot.com/4172049/diff/5003/src/cmd/gc/const.c
File src/cmd/gc/const.c (right):

http://codereview.appspot.com/4172049/diff/5003/src/cmd/gc/const.c#newcode1058
src/cmd/gc/const.c:1058: s1 = (uchar *)l->val.u.sval->s;
make this look like the other casts in this program: (uchar*)

[jeff.allen, 2011-02-16 21:32:15]

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

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

[rsc1, 2011-02-16 22:46:10]

LGTM

http://codereview.appspot.com/4172049/diff/3004/test/fixedbugs/bug1515.go
File test/fixedbugs/bug1515.go (right):

http://codereview.appspot.com/4172049/diff/3004/test/fixedbugs/bug1515.go#new...
test/fixedbugs/bug1515.go:16: if ((s1<s2) != (joao<jose)) {
i will drop outer parens before submitting
i thought gofmt did this already

[rsc, 2011-02-16 22:57:24]

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

gc: make string const comparison unsigned

Make compile-time string const comparison match semantics
of runtime.cmpstring.

Fixes issue 1515.

R=rsc
CC=golang-dev, rog
http://codereview.appspot.com/4172049

Committer: Russ Cox <rsc@golang.org>