Open Bug 900661 Opened 7 years ago Updated 6 years ago

Clean up excessive implicit 64>32 conversions warnings on 64-bit platforms due to 64-bit size_t

Categories

(Core :: XPCOM, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

People

(Reporter: vlad, Unassigned)

Details

Attachments

(1 file)

The last argument of NS_LogAddRef is a uint32_t, but on 64-bit platforms, size_t is 64-bit.  This is most commonly invoked via NS_LOG_ADDREF(...., sizeof(*this)), so we end up with a conversion from uint64_t -> uint32_t, triggering compilation warnings (msvc at least).

I can fix this in two ways -- try to change NS_LogAddRef and below to use size_t.  This would be an ABI change on 64-bit platforms.  I can also add a static_cast<> in NS_LOG_ADDREF.  Thoughts?
NS_LogAddRef should only be called in trace-refcnt builds, right?  It shouldn't matter that much, but my preference is to keep the ABI the same, and just use a static_cast.

David should probably be the right person to give you a green light here.
Flags: needinfo?(dbaron)
I'd probably be inclined to change to size_t -- I didn't think we were worrying about ABI compatibility anymore, and I'm even less worried for extensions compiled with -DDEBUG.
Flags: needinfo?(dbaron)
Morphing so I can just attach a pile of small patches.

The biggest culprit of this by far is nsTArray code.  We have lots of code that stores the result of .Length() or takes numeric args in the form of size_t, and then passes it to arrays that are indexed by uint32_t.  e.g., this is common:

size_t size = someArray.Length();
for (size_t i = 0; i < size; ++i) {
  doSomething(someArray[i]);
}

this warns on the array index in doSomething, since someArray's operator[] takes the array's index_type, which will often be uint32_t.  Playing whackamole with this isn't hard, but is annoying; though I don't have a better idea.
Summary: NS_LogAddRef takes uint32_t as classSize instead of size_t → Clean up excessive implicit 64>32 conversions warnings on 64-bit platforms due to 64-bit size_t
A lot of the warnings stem from nsTArray having an operator[] (and related) that takes index_type, and index_type more often than not being uint32_t.  Then we have lots of code that uses size_t as indexes.

Here's a sample of the kinds of changes I've made so far.  A lot of it is converting code that's using size_t to using uint32_t.  This is kind of crappy, though perhaps more correct; if the arrays ever start using a 64-bit index though, the warnings will come back, and they'll be real issues this time around.

There's a smattering of other things as well; feedback encouraged.  (Note, the tracerefcnt thing ended up being a pain to turn into size_t; if someone wants to do it, they can go for it, but given that it's just tracerefcnt, I figured we can live with a cast.)

(Why am I doing this? OCD. With ninja my builds fast, so that I can actually make core include file changes and see what happens in finite time, and I can also remove lots of noise while building.)
Attachment #785472 - Flags: feedback?(ehsan)
Attachment #785472 - Flags: feedback?(dbaron)
Attachment #785472 - Flags: feedback?(bzbarsky)
Comment on attachment 785472 [details] [diff] [review]
example of the types of changes

There's been talk of making index_type be size_t too, actually...
Attachment #785472 - Flags: feedback?(bzbarsky) → feedback+
I'm inclined to think we ought to move towards making anything that represents indices into arrays, sizes in memory, or reference counts, use a size_t.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #6)
> I'm inclined to think we ought to move towards making anything that
> represents indices into arrays, sizes in memory, or reference counts, use a
> size_t.

+1
Comment on attachment 785472 [details] [diff] [review]
example of the types of changes

I think that's a bit of consensus on going the other direction, then, although we might want to discuss a little more widely in case somebody's going to put their foot down on that way.
Attachment #785472 - Flags: feedback?(dbaron) → feedback-
Nod; I should start a dev.platform thread.  I took a quick look at changing things to use size_t at the root though, and it felt non-trivial.
Comment on attachment 785472 [details] [diff] [review]
example of the types of changes

Review of attachment 785472 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, let's switch nsTArray to use size_t.  IIRC bjacob started such a thread a while ago, not sure what happened as a result.
Attachment #785472 - Flags: feedback?(ehsan) → feedback-
Flags: needinfo?(bjacob)
Yes, all indices / sizes should be size_t unless there is a very specific reason not to. Indeed, the C and C++ languages themselves want sizes to be size_t, e.g. that is what sizeof returns.

In the case of nsTArray, I suspect that we may want to keep the internal nsTArrayHeader storage use 32-bit integers for size and capacity (although the optimization of empty arrays not even having a header, might make that moot), but everything else --- the entire nsTArray API, and all the code that iterates over an array --- should use size_t.

Note that in some cases this is not just shutting up warnings, this can actually be an optimization on 64bit systems, as using 32bit integers as array offsets can require the compiler to emit extra instructions to expand those into 64bit integers and clear the top 32 bits.
Flags: needinfo?(bjacob)
Filed bug 1004098 to fix the interface of nsTArray.
You need to log in before you can comment on or make changes to this bug.