Closed Bug 860457 Opened 8 years ago Closed 8 years ago

use uintptr_t instead of unsigned long as our "unsigned integer with the same size as a pointer", in celldata.h

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(1 file)

No description provided.
The right thing will happen everywhere except for wacky 64-bit
big-endian platforms with 64-bit pointers and 32-bit longs, I think.
But we should still fix it. :)
Attachment #735941 - Flags: review?(dholbert)
Comment on attachment 735941 [details] [diff] [review]
use uintptr_t instead of unsigned long in celldata.h

Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )

You could do this in the CellData constructor, in nsCellMap.cpp.

r=me with that, assuming this compiles. (i.e. assuming it passes a Try build on all platforms.  I've been bitten by missing-uintptr_t-support in the past, I think, though maybe we've got it defined in a header somewhere now.)
Attachment #735941 - Flags: review?(dholbert) → review+
Summary: use uintptr_t instead of unsigned long in celldata.h → use uintptr_t instead of unsigned long as our "unsigned integer with the same size as a pointer", in celldata.h
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there
> actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )

uintptr_t is defined to be large enough to hold a pointer, so I'm not sure that the assertion is really checking anything useful.

> r=me with that, assuming this compiles. (i.e. assuming it passes a Try build
> on all platforms.  I've been bitten by missing-uintptr_t-support in the
> past, I think, though maybe we've got it defined in a header somewhere now.)

uintptr_t is provided in <stdint.h> on Mac, Linux, and some versions of Windows.  For those versions of Windows where it's not, we have mfbt/MSStdInt.h to fill in the gap.
(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Daniel Holbert [:dholbert] from comment #2)
> > Could you add a MOZ_STATIC_ASSERT to ensure that the assumption there
> > actually holds?  (i.e. check that sizeof(mOrigCell) == sizeof(mBits) )
> 
> uintptr_t is defined to be large enough to hold a pointer, so I'm not sure
> that the assertion is really checking anything useful.

It is useful. It'd be:
 (a) documenting / verifying our expectation that those variables have the same size
 (b) verifying that we're using a sane uintptr_t definition, and we're not e.g. accidentally getting the wrong typedef from another header which is stomping on mfbt's typedef. (Note that we have 12 different  "typedef [something]  uintptr_t;" statements in the tree, with a variety of different [something] values.)
(hopefully any of the typedefs we might get would be the right size, but there's no harm in explicitly checking it to be on the safe side)
(In reply to Daniel Holbert [:dholbert] from comment #4)
>  (b) verifying that we're using a sane uintptr_t definition, and we're not
> e.g. accidentally getting the wrong typedef from another header which is
> stomping on mfbt's typedef. (Note that we have 12 different  "typedef
> [something]  uintptr_t;" statements in the tree, with a variety of different
> [something] values.)

Bleh.  All right, you've convinced me.  (I think all those definitions are correct, but you're right, we might as well verify.)
https://hg.mozilla.org/mozilla-central/rev/efaa46a0e3e6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.