239499 - 64bit safe code for mozilla/content (WinXP AMD64) part 2

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219

sAttrValue and nsAttrAndChildArray uses unsigned long as pointer.  We need to
change it to PRUword for porting of WinXP 64bit.  About it, please see bug 229722.

Reproducible: Always
Steps to Reproduce:
not need.  This is porting issue
Actual Results:  
not need.  This is porting issue

Expected Results:  
not need.  This is porting issue

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch of mozilla/content at 2004/04/03

Comment 2

[Roland Mainz]

Nice catch...

Comment 3

[Roland Mainz]

Comment on attachment 145370 [details] [diff] [review]
patch of mozilla/content at 2004/04/03

Requesting r= from pkw since he has a 64bit platform around to test.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Um... ccing module owners and such so they have a clue as to what's going on in
their module.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 145370 [details] [diff] [review]
patch of mozilla/content at 2004/04/03

Please remove the typedefs entierly and use the PRUword type directly instead.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: patch for mozilla/content

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can you not use PRWord?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: centralize PtrBits

This should be a more stable approach. There is clearly a need for this type across our codebase, so lets create a proper define in XPCOM that everyone can use.

We should consider removing this type across the tree once NSPR has a proper pointer-sized integer type. However even then it might make sense to have a type that is specifically for use in bitfields (for one so that people don't forget to make it unsigned).

Comment 9

[Darin Fisher]

Comment on attachment 217225 [details] [diff] [review]
centralize PtrBits

>Index: xpcom/base/nscore.h

>+/**
>+ * Generic type for a bitfield that is the size of a void*
>+ *
>+ * NOTE! This define will change or go away once NSPR provides a
>+ * real datatype for integers whos size is the size of a void*,
>+ * but it is the recommended type until then.
>+ */
>+typedef PRUword PtrBits;
>+

Couple comments:

1) strange for this type to not begin with "ns" or "PR".. nscore.h is included by anyone trying to use XPCOM so we have to be sensitive about conflicting with user-defined types.  namespacing our types, defines, and symbols probably matters a lot.

2) nspr seems to define the type.. PRUword, no?  at least, this code seems inconsistent with the comment.

In short, I'm not sure that we want exactly this added to nscore.h :(

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #9)
> 2) nspr seems to define the type.. PRUword, no?  at least, this code seems
> inconsistent with the comment.

For two reasons:
1. PR(U)word have a big comment at the top of it saying not to use this type. Why i'm not really sure and atm there is no better alternative. See bug 313297, bug 229722.

2. Even with PR(U)word around people seem to have felt it neccesary to create a new define, as proven by the fact that most of the defines point to PRUword.

So I do think we want a special define, it's just a matter of where it should live and what it should be named. And I'd prefer not to wait for nspr.


> 1) strange for this type to not begin with "ns" or "PR".. nscore.h is included
> by anyone trying to use XPCOM so we have to be sensitive about conflicting
> with user-defined types.  namespacing our types, defines, and symbols probably
> matters a lot.

Good point.. half of the idea with this apprach was to not have to go through the tree and mess up the blame twice, once now and once if/when NSPR get their act together.

But it's not so bad so maybe i should just rename it nsptrbits?

Comment 11

[Darin Fisher]

Let's just fix bug 313297.  Propose a patch in that bug :-)

Comment 12

[Darin Fisher]

Comment on attachment 217225 [details] [diff] [review]
centralize PtrBits

ok, minusing this patch in favor of nspr solution.  thanks!

Comment 13

[Edward Lam]

Does this mean we should add bug 313297 as a dependency of bug 237202 as well then?

Comment 14

[Makoto Kato [:m_kato]]

Yes, we should do it.

Comment 15

[Makoto Kato [:m_kato]]

Attachment: patch v3

same as bug 478190.  This part is for mozilla/content

Comment 16

[Makoto Kato [:m_kato]]

landed
http://hg.mozilla.org/mozilla-central/rev/66c661e575a9