Closed
Bug 989123
Opened 11 years ago
Closed 11 years ago
Add EnumeratedArray class to MFBT for plain static arrays indexed by a typed enum
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(1 file, 3 obsolete files)
2.95 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #8398191 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
Refreshed with new constructors that were very useful on content/canvas code that I was porting.
Attachment #8398191 -
Attachment is obsolete: true
Attachment #8398191 -
Flags: review?(jwalden+bmo)
Attachment #8398490 -
Flags: review?(jwalden+bmo)
Comment 3•11 years ago
|
||
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>.
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Comment 5•11 years ago
|
||
(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.
Assignee | ||
Comment 6•11 years ago
|
||
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...).
Assignee | ||
Comment 7•11 years ago
|
||
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.
Attachment #8398490 -
Attachment is obsolete: true
Attachment #8398490 -
Flags: review?(jwalden+bmo)
Attachment #8399301 -
Flags: review?(jwalden+bmo)
Comment 8•11 years ago
|
||
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.
Attachment #8399301 -
Flags: review?(jwalden+bmo) → feedback+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8406176 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•11 years ago
|
Attachment #8399301 -
Attachment is obsolete: true
Comment 10•11 years ago
|
||
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.
Attachment #8406176 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 11•11 years ago
|
||
[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
Assignee | ||
Comment 12•11 years ago
|
||
Assignee: nobody → bjacob
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•