Closed Bug 1325627 Opened 4 years ago Closed 4 years ago
Add sentinel to WR enums that go over IPC
WebRenderTypes.h has IPC serializations for enums in webrender.h. However the enums don't have sentinel values at the end, so the last "real" value is used in the IPC serialization definitions, e.g. at https://hg.mozilla.org/projects/graphics/file/7b88f8d64a95/gfx/layers/wr/WebRenderTypes.h#l89. This means that if somebody adds a new value to the end of the enum they might forget to update the IPC serialization, causing hard-to-track-down errors.
There is a downside to adding a sentinel value, as Seth pointed out in bug 1266301: it prevents the compiler from giving accurate exhaustiveness warnings for switch statements over the enum. I don't have a great solution for this, just pointing it out so we're aware of the tradeoffs.
(In reply to Botond Ballo [:botond] from comment #1) > There is a downside to adding a sentinel value, as Seth pointed out in bug > 1266301: it prevents the compiler from giving accurate exhaustiveness > warnings for switch statements over the enum. This can be worked around by putting the sentinel in the switch and just MOZ_RELEASE_ASSERT(false)'ing it. It's not perfect but we do it in a bunch of places already. This bug should be fixed ASAP because we're actually doing the wrong thing now, like creating ContiguousEnumSerializer with valid values as the third template argument.
Assignee: nobody → bugmail
Also WrTextureFilter is defined as an "enum class" inside a "extern C" header, which is probably not good, as the compiler may do things that make it incompatible with rust's C bindings.
(... but making it a regular enum means "Point" is in the global namespace and it causes compilation failures in faraway places that are trying to use gfx::Point)
Hm. I don't know what the best solution is. Would declaring it |enum WrTextureFilter : int|, force it to be compatible with C? Or maybe char because we only have two values, so I don't think the enum would be that big. Either way that is kind of messy. I'd be fine with making it a normal enum and prefixing the enum values with 'WR_TEXFILTER_' or however we normally do it in Gecko.
Botond said that "enum class" *should* in practice be the same size as "enum", so I think it might be easier to convert them all to "enum class" (to avoid namespacing ugliness) and then add a static_assert inside an #ifdef DEBUG block to make sure the sizes wind up the same. I'm working on a patch that does that now.
Pushed by email@example.com: https://hg.mozilla.org/projects/graphics/rev/f20aca8fdea2 Ensure all serialized Wr* enums have sentinel values and are consistently defined as |enum class|. r=gfx?
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
4 years ago
See Also: → 1332737
4 years ago
Target Milestone: --- → mozilla54
Comment on attachment 8828924 [details] [diff] [review] Add sentinel values and consistently use enum classes Review of attachment 8828924 [details] [diff] [review]: ----------------------------------------------------------------- I'm just going to r+ this since a lot of people have looked at and further modified the code in question since this landed, and nobody seems to be keeping an eye on review requests to :gfx.
Attachment #8828924 - Flags: review?(graphics-team) → review+
You need to log in before you can comment on or make changes to this bug.