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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
a month ago

People

(Reporter: m_kato, Assigned: sicking)

Tracking

({64bit})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

15 years ago
(Reporter)

Updated

15 years ago
Blocks: 237202

Comment 2

15 years ago
Nice catch...
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

15 years ago
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.
Attachment #145370 - Flags: review?(pkw)

Updated

15 years ago
Keywords: 64bit
Um... ccing module owners and such so they have a clue as to what's going on in
their module.
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.
Attachment #145370 - Flags: review?(pkw) → review-
(Reporter)

Comment 6

15 years ago
Posted patch patch for mozilla/content (obsolete) — Splinter Review
Attachment #145370 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #169010 - Flags: review?
(Reporter)

Updated

15 years ago
Assignee: general → m_kato
(Reporter)

Updated

15 years ago
Status: NEW → ASSIGNED

Updated

13 years ago
Attachment #169010 - Flags: review? → review?(bugmail)
Can you not use PRWord?
Posted patch centralize PtrBits (obsolete) — Splinter Review
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).
Assignee: m_kato → bugmail
Attachment #169010 - Attachment is obsolete: true
Attachment #217225 - Flags: superreview?(darin)
Attachment #217225 - Flags: review?(darin)
Attachment #169010 - Flags: review?(bugmail)

Comment 9

13 years ago
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 :(
(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

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

Comment 12

13 years ago
Comment on attachment 217225 [details] [diff] [review]
centralize PtrBits

ok, minusing this patch in favor of nspr solution.  thanks!
Attachment #217225 - Flags: superreview?(darin)
Attachment #217225 - Flags: superreview-
Attachment #217225 - Flags: review?(darin)
Attachment #217225 - Flags: review-

Comment 13

13 years ago
Does this mean we should add bug 313297 as a dependency of bug 237202 as well then?
(Reporter)

Comment 14

13 years ago
Yes, we should do it.
Blocks: 313297
(Reporter)

Updated

13 years ago
No longer blocks: 313297
(Reporter)

Updated

11 years ago
(Reporter)

Comment 15

10 years ago
Posted patch patch v3Splinter Review
same as bug 478190.  This part is for mozilla/content
Attachment #217225 - Attachment is obsolete: true
Attachment #370577 - Flags: review?(peterv)
Attachment #370577 - Flags: superreview+
Attachment #370577 - Flags: review?(peterv)
Attachment #370577 - Flags: review+
(Reporter)

Comment 16

10 years ago
landed
http://hg.mozilla.org/mozilla-central/rev/66c661e575a9
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.