989033 - Remove EnumSet from MFBT

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: remove EnumSet

EnumSet seems like a good idea from the Java world, and there seems to be precedent for adapting it in C++.

However:

1. There are no users of EnumSet in mozilla-central at the moment.

2. Bug 987290 allows using Typed Enums directly as bitflags, which offers similar features to EnumSet, with additional benefits (drop-in upgrade for all the |& bitflags that we have in gecko, storage as a POD enum value instead of as a class object, literal representation, existing IPC serialization code).

So let's remove EnumSet.

Comment 1

[Jeff Walden [:Waldo]]

kentuckyfriedtakahe seems not to have landed his use for this yet in bug 793013.  You guys can cagefight over whether this lack of use will or won't be permanent.  :-)

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ah, interesting. So what we have here is an enum, BackendType in gfx/2d/Types.h, that is used to index a bitfield. That is indeed exactly what EnumSet does.

But since this is only being considered for one enum at the moment, maybe it's worth considering ways to do this slightly differently, that don't require a new concept.

For example, with bug 987290, one could introduce a new enum, BackendTypeFlags, with values that would be 1 << the BackendType values; write a helper function doing the 1 << conversion from BackendType to BackendTypeFlags; and the we'd be back to plain typed enum bitflags.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I don't have a strong feeling about it. Feel free to remove EnumSet if you think you've got it otherwise covered. I just don't like the 1 << x pattern but I'm no longer working on gfx code.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8398055 [details] [diff] [review]
remove EnumSet

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

Ixnay away-ay.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Good thing that I haven't landed this yet: bug 950312 is introducing a use case of EnumSet that is using it exactly for what it's better at, than my alternative. See bug 950312 comment 29.

WONTFIX'ing for now. Will try to think of a good way to handle this with just typed enums, and will revisit if I find one.