400393 - mozilla-central: jsatom.cpp has bad casts in 64-bit OS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Smedberg]

jsatom.cpp has bad casts on a 64-bit OS: you aren't allowed to cast a pointer directly to a uint32... you always have to go through a uintN first.

Comment 1

[Brendan Eich [:brendan]]

JS_PTR_TO_{,U}INT32 stand ready. Where are these errors?

Igor, can you fix?

/be

Comment 2

[Benjamin Smedberg]

Attachment: Make it cast correctly, rev. 1

This is the patch I thought I attached the first time... :-(

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 285485 [details] [diff] [review]
Make it cast correctly, rev. 1

Right-justify backslash to column 79 for prevailing style match.

/be

Comment 4

[Igor Bukanov]

Comment on attachment 285485 [details] [diff] [review]
Make it cast correctly, rev. 1

>diff --git a/js/src/jsatom.h b/js/src/jsatom.h
>--- a/js/src/jsatom.h
>+++ b/js/src/jsatom.h
>@@ -74,7 +74,7 @@ JS_STATIC_ASSERT(sizeof(JSAtom *) == JS_
> #if JS_BYTES_PER_WORD == 4
> # define ATOM_HASH(atom)          ((JSHashNumber)(atom) >> 2)
> #elif JS_BYTES_PER_WORD == 8
>-# define ATOM_HASH(atom)          (((JSHashNumber)(atom) >> 3) ^              \
>+# define ATOM_HASH(atom)          (((JSHashNumber)(jsuword)(atom) >> 3) ^ \
>                                    (JSHashNumber)((jsuword)(atom) >> 32))

Nit: put \ in the column 78 as it was before the patch.

r+ with that fixed.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> (From update of attachment 285485 [details] [diff] [review])
> >diff --git a/js/src/jsatom.h b/js/src/jsatom.h
> >--- a/js/src/jsatom.h
> >+++ b/js/src/jsatom.h
> >@@ -74,7 +74,7 @@ JS_STATIC_ASSERT(sizeof(JSAtom *) == JS_
> > #if JS_BYTES_PER_WORD == 4
> > # define ATOM_HASH(atom)          ((JSHashNumber)(atom) >> 2)
> > #elif JS_BYTES_PER_WORD == 8
> >-# define ATOM_HASH(atom)          (((JSHashNumber)(atom) >> 3) ^              \
> >+# define ATOM_HASH(atom)          (((JSHashNumber)(jsuword)(atom) >> 3) ^ \
> >                                    (JSHashNumber)((jsuword)(atom) >> 32))
> 
> Nit: put \ in the column 78 as it was before the patch.

I meant 79 in editors that counts columns from 1.

Comment 6

[Benjamin Smedberg]

Pushed to mozilla-central. Igor, could you go ahead and set the review flag when you do reviews so that my review requests stay clean?

Comment 7

[Brendan Eich [:brendan]]

This should get patched in cvs.mozilla.org too.

/be

Comment 8

[Benjamin Smedberg]

Why fix in CVS? This isn't a problem in C, only in C++ due to different casting rules.

Comment 9

[Igor Bukanov]

(In reply to comment #6)
> Pushed to mozilla-central. Igor, could you go ahead and set the review flag
> when you do reviews so that my review requests stay clean?

Do you have a patch to attach with the nit addressed?

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> Why fix in CVS? This isn't a problem in C, only in C++ due to different casting
> rules.

Two reasons:

1. To allow 1.9 and mozilla-central to stay as close as possbile.

2. To allow to use C++ compiler with 1.9.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Why fix in CVS? This isn't a problem in C, only in C++ due to different casting
> rules.

Igor replied, but did not cite bug 357016 which was fixed on cvs.m.o in reason 2. Connect the dots, and don't make gratuitous small changes between SpiderMonkey in cvs and hg, please.

/be

Comment 12

[Benjamin Smedberg]

Attachment: Make it cast correctly, rev. 1.1

New patch... though if you're going to say r+ in comments with a nit changed, you might as well mark it so.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Created an attachment (id=285501) [details]
> Make it cast correctly, rev. 1.1
> 
> New patch...

Thanks, I just wanted to have a patch to apply to 1.9 as well. Do you have CVS access to commit the patch (after getting a+)?

Comment 14

[Reed Loden [:reed]]

Checking in js/src/jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.77; previous revision: 3.76
done