This will give us strict typing on array indices. And, compared to a plain array, it has the extra side benefit of adding the out-of-bounds-access assertion. I have a few use cases already, see dependent bugs.

Note that the need for this also increases when we convert an existing plain enum into a MFBT typed enum, as (without EnumeratedArray) we then have to add ugly manual casts to size_t when addressing the array.

Refreshed with new constructors that were very useful on content/canvas code that I was porting.

Comment on attachment 8398490 [details] [diff] [review] EnumeratedArray Review of attachment 8398490 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/EnumeratedArray.h @@ +22,5 @@ > +public: > + static const size_t Size = size_t(SizeAsEnumValue); > + > +private: > + ValueType mArray[Size]; Seems like you should use: Array<ValueType, Size> mArray and then you would get the bounds checking currently in OffsetForIndex for free. Or even inherit (non-publically, I think) from Array<ValueType, Size>.

(In reply to Nathan Froyd (:froydnj) from comment #3) > Seems like you should use: > > Array<ValueType, Size> mArray > > and then you would get the bounds checking currently in OffsetForIndex for > free. Or even inherit (non-publically, I think) from Array<ValueType, Size>. Ah, thanks, I didn't know about this Array class. Do you have an opinion about the ordering of template parameters, and how to keep it sane wrt Array? Currrently we have Array<ValueType, Size> EnumeratedArray<IndexType, Size, ValueType> The mismatch in ordering of Size vs ValueType is unfortunate, but I don't know how to fix it. I would like Size to remain adjacent to IndexType as it is related to it; and it seems strange to have IndexType after ValueType, but maybe that's what I should get over.

(In reply to Benoit Jacob [:bjacob] from comment #4) > Currrently we have > > Array<ValueType, Size> > EnumeratedArray<IndexType, Size, ValueType> > > The mismatch in ordering of Size vs ValueType is unfortunate, but I don't > know how to fix it. I would like Size to remain adjacent to IndexType as it > is related to it; and it seems strange to have IndexType after ValueType, > but maybe that's what I should get over. /me wishes yet again for compile-time reflection on enums. I vote for ValueType, Size, IndexType, I think, though I guess the compiler is probably going to want ValueType, IndexType, Size so that Size can be properly typed. ValueType, Size, IndexType would be especially nice if EnumeratedArray could just be a special case of Array, but I don't see how to make that happen...maybe very liberal use of EnableIf.

Indeed, I don't see a way that Size could precede IndexType. Maybe in a future world where we would have automatic template parameter type deduction from template parameters (right now we only get it from ordinary function parameters...).

Now using MFBT Array, so we don't need to repeat the MOZ_ASSERTs here. But it's not through inheritance, because it wasn't convenient. Public inheritance doesn't make this simpler, as we don't want to expose the operator[] taking plain integers. It just seemed more appropriate to use Array as the type of the mArray member.

Comment on attachment 8399301 [details] [diff] [review] EnumeratedArray Review of attachment 8399301 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/EnumeratedArray.h @@ +13,5 @@ > +#include "mozilla/TypedEnum.h" > + > +namespace mozilla { > + > +template <typename IndexType, Needs an overview documentation comment with an example in it, like the other mfbt classes have. @@ +16,5 @@ > + > +template <typename IndexType, > + MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(IndexType) SizeAsEnumValue, > + typename ValueType> > +class EnumeratedArray Class body should be indented another two spaces. @@ +18,5 @@ > + MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(IndexType) SizeAsEnumValue, > + typename ValueType> > +class EnumeratedArray > +{ > + No blank line here, please. @@ +32,5 @@ > + explicit EnumeratedArray(const EnumeratedArray& aOther) > + { > + for (size_t i = 0; i < Size; i++) { > + mArray[i] = aOther.mArray[i]; > + } No braces on these for-loops.

Comment on attachment 8406176 [details] [diff] [review] EnumeratedArray Review of attachment 8406176 [details] [diff] [review]: ----------------------------------------------------------------- r+, but note the substantive issue we should resolve before landing this. I'll ping you on IRC. ::: mfbt/EnumeratedArray.h @@ +15,5 @@ > +namespace mozilla { > + > + > +/** > + * EnumeratedArray is an fixed-size array container for use when an a fixed-size @@ +33,5 @@ > + * Sheep, > + * Count > + * MOZ_END_ENUM_CLASS(AnimalSpecies) > + * > + * EnumeratedArray<AnimalSpecies, AnimalSpecies::Count, int> headCount; So, looking at this example, I wonder. What do you think about anointing one particular initializer (Count, Limit, Size, something?) as the canonical count for the enum? Then people only have to specify Count in the enum, and it'll automatically be picked up. That is, mod typos: template<typename IndexName, typename ValueType> class EnumeratedArray { public: static const size_t Size = size_t(IndexType::Count); ... }; This does force people to include the count within the enum (theoretically they could pass AnimalSpecies(2) here, although I'd think most reviewers would r- that). That might not always be desirable, if any code ever switches on the enum and wants to take advantage of incomplete-coverage compiler warnings. This seems possibly like a rare case, tho, that we should live with here. What do you think? (Tihs is one reason examples and docs are great -- something was tickling my mind about this that I couldn't articulate before, but the example it made much clearer to me.) @@ +39,5 @@ > + * headCount[AnimalSpecies::Cow] = 17; > + * headCount[AnimalSpecies::Sheep] = 30; > + * > + */ > +template <typename IndexType, I seem to recall there shouldn't be a space between "template" and "<" to be consistent with other code.

[16:38] <bjacob_> i see the point of what you're proposing. One less template parameter, that's nice. I wonder, though, if hardcoding a particular enum value identifier is the right way. For example, people might already have a differently named enum value for that, in a preexisting enum. Unless we can magically get everyone to agree, this will mean redundant enum values, prone to bugs giving them the wrong numeric value [16:38] <bjacob_> e.g. [16:38] <bjacob_> enum class MyEnum { A, B, Max, Count=Max }; [16:38] <bjacob_> so [16:38] <bjacob_> if we wanted to make it automagic as you suggest, [16:39] <bjacob_> i would then rather have it done by a traits kind of helper [16:40] <bjacob_> hm though, that would be verbose and also prone to forgetting to update it when a new value is inserted [16:40] <bjacob_> i think i would be in favor of not changing the current approach for now. At least it's down-to-earth. [16:44] <Waldo> fine enough, was just mooting the idea :-) [16:44] <Waldo> carry on