1266301 - Use a nicer serialization mechanism for VisibilityCounter values

Status: NEW

(Core :: Layout, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now we're serializing VisibilityCounter values in VisibilityIPC.h with a simple cast to a uint8_t. This doesn't give us type safety when casting back, which is not good. However, the alternative we have right now also isn't good:

https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/ipc/glue/IPCMessageUtils.h#140

This solution prevents the compiler from giving accurate exhaustiveness warnings for switch statements, which is a pretty big deal to me.

The best fix I can see for this is to add an EnumRange traits class that lets you define the first and last values of the enum right now to it, without polluting the value space of the enum itself.

Comment 1

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> This solution prevents the compiler from giving accurate exhaustiveness
> warnings for switch statements, which is a pretty big deal to me.

In case it's not clear, this is because you need to design your enums like this:

enum Foo {
  FOO_A,
  FOO_B,
  NUM_FOO
};

and ContiguousEnumSerializer<Foo, FOO_A, NUM_FOO> will check that values are >= FOO_A and < NUM_FOO. That means that inherently, NUM_FOO cannot be a valid value of Foo (and you can't serialize it!) and hence you'd have to either awkwardly handle it in every switch statement involving Foo or add |default| clauses that would mask failures to handle all cases.

Comment 2

[Botond Ballo [:botond]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> The best fix I can see for this is to add an EnumRange traits class that
> lets you define the first and last values of the enum right now to it,
> without polluting the value space of the enum itself.

Could we simply define a new subclass of EnumSerializer, called e.g. "ContiguousEnumSerializedInclusive", which was like ContiguousEnumSerializer, but treated the upper bound as the last legal value, rather than as a sentinel value?

Comment 3

[Seth Fowler [:seth] [:s2h]]

(In reply to Botond Ballo [:botond] from comment #2)
> Could we simply define a new subclass of EnumSerializer, called e.g.
> "ContiguousEnumSerializedInclusive", which was like
> ContiguousEnumSerializer, but treated the upper bound as the last legal
> value, rather than as a sentinel value?

Yeah, we could. I honestly can't remember why I thought a traits class was better; probably it was just because I thought we'd have other uses for this information in other places and defining it a single time in a traits class would be convenient.