PSA: nsTArray lengths and indices are now size_t (were uint32_t)

[Benoit Jacob]

Hi,

Since Bug 1004098 landed, the type of nsTArray lengths and indices is now
size_t.

Code using nsTArrays is encouraged to use size_t for indexing them; in most
cases, this does not really matter; however there is one case where this
does matter, which is when user code stores the result of
nsTArray::IndexOf().

Indeed, nsTArray::NoIndex used to be uint32_t(-1), which has the value 2^32
- 1. Now, nsTArray::NoIndex is size_t(-1) which, on x86-64, has the value
2^64 - 1.

This means that code like this is no longer correct:

uint32_t index = array.IndexOf(thing);

Such code should be changed do:

size_t index = array.IndexOf(thing);

Or, better still (slightly pedantic but would have been correct all along):

ArrayType::index_type index = array.IndexOf(thing);

Where ArrayType is the type of that 'array' variable (one could use
decltype(array) too).

Thanks,
Benoit

[Ehsan Akhgari]

Do you think it's worth trying to make the bad code above not compile,
by returning an object from IndexOf which only provides an implicit
conversion to size_t and not uint32_t? Like:

template <class T>
class nsTArray {
// ...
struct Size {
Size(size_t);
operator size_t() const;
};
Size IndexOf(...);
};

[Benoit Jacob]

Yes, absolutely, it is worth catching this at compile-time.

But what you describe above wouldn't catch that at compile-time, because
the compiler would use this operator to get an implicit conversion to
size_t followed by implicit conversion from size_t to uint32_t.

You could use MFBT's new MOZ_EXPLICIT_CONVERSION to annotate this
conversion operator with 'explicit' on supporting C++11 compilers. But
instead, I would not bother trying to make this a conversion operator, I
would just make this a plain old getter method.

I would also not call that Size, but IndexOfResult. The problem is that
what IndexOf() returns is not necessarily a "size" or "index", it may also
be this special thing called "NoIndex". So I don't think that its return
type should "feel like" it _is_ just an index. Instead, its return type
should be something that is _either_ an index or NoIndex.

Something like this:

class IndexOfResult {

const index_type mIndex;
static const index_type NoIndex = index_type(-1);

public:

IndexOfResult(index_type index) : mIndex(index) {}
IndexOfResult(const IndexOfResult& other) : mIndex(other.mIndex) {}

bool Found() const {
return mIndex != NoIndex;
}

index_type Value() const {
MOZ_ASSERT(Found());
return mIndex;
}
};

Benoit