code review 5486049: gc: do not use vlong in switch expressions

gc: do not use vlong in switch expressions

This fails when using 8c on Plan 9 since
switch expressions must be no larger than
the machine's word size.

[ality, 2011-12-13 13:51:49]

Hello lucio.dere@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[lucio, 2011-12-13 14:13:54]

LGTM. I'm curious to know whether I'm being careless or Anthony is being too
cautious (just for future reference).

Lucio.

http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/reflect.c#newcode977
src/cmd/gc/reflect.c:977: if(t->width <= 16) {
I don't think it's necessary to go to this extreme.  Merely casting to int
suffices, any much bigger value will be caught by the default rule.  Later,
there is no default rule and no action outside of the given sizes is required. 
My recommendation is simply to cast these, not to check them asy more carefully.

http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/subr.c#newcode2647
src/cmd/gc/subr.c:2647: if(size <= 16) {
Overkill, as I explained earlier.

[ality, 2011-12-13 14:24:20]

lucio.dere@gmail.com once said:
>
http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/reflect.c#newcode977
> src/cmd/gc/reflect.c:977: if(t->width <= 16) {
> I don't think it's necessary to go to this extreme.  Merely casting to
> int suffices, any much bigger value will be caught by the default rule.
> Later, there is no default rule and no action outside of the given sizes
> is required.  My recommendation is simply to cast these, not to check
> them asy more carefully.

Not true.  What happens when you cast the
vlong 0x0000000100000010 (4294967312) to
an int?  It becomes 0x10

Sure you would need an enormous struct or
array to hit this (at least 2^32+16 bytes)
but I'd rather be on the safe side.

http://codereview.appspot.com/5486049/

[lucio, 2011-12-13 14:31:16]

I thought I was the most pedantic person I knew :-)

Thing is, and Russ can arbitrate, these are connected to comparisons
between complicated data objects.  If these objects are 4GiB or
greater, comparisons make little sense and I would vote in favour of a
bit of speed rather than cater for extreme behaviour.

Still, I appreciate your caution.

Lucio.

On 12/13/11, Anthony Martin <ality@pbrane.org> wrote:
> lucio.dere@gmail.com once said:
>>
http://codereview.appspot.com/5486049/diff/3001/src/cmd/gc/reflect.c#newcode977
>> src/cmd/gc/reflect.c:977: if(t->width <= 16) {
>> I don't think it's necessary to go to this extreme.  Merely casting to
>> int suffices, any much bigger value will be caught by the default rule.
>> Later, there is no default rule and no action outside of the given sizes
>> is required.  My recommendation is simply to cast these, not to check
>> them asy more carefully.
>
> Not true.  What happens when you cast the
> vlong 0x0000000100000010 (4294967312) to
> an int?  It becomes 0x10
>
> Sure you would need an enormous struct or
> array to hit this (at least 2^32+16 bytes)
> but I'd rather be on the safe side.
>
> http://codereview.appspot.com/5486049/
>

[ality, 2011-12-13 15:05:48]

Hello lucio.dere@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-12-13 20:16:15]

How about fixing the Plan 9 C compiler instead?
Try http://codereview.appspot.com/5485063

[r2, 2011-12-13 20:33:39]

On Dec 13, 2011, at 12:16 PM, Russ Cox wrote:

> How about fixing the Plan 9 C compiler instead?
> Try http://codereview.appspot.com/5485063

Yes, do that. It's standard C.

-rob

[ality, 2011-12-14 06:45:50]

*** Abandoned ***