Closed
Bug 1325627
Opened 8 years ago
Closed 8 years ago
Add sentinel to WR enums that go over IPC
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox53 | --- | affected |
People
(Reporter: kats, Assigned: kats)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file)
5.04 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
(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
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
(... 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)
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8828924 -
Flags: review?(graphics-team)
Pushed by kgupta@mozilla.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: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•8 years ago
|
Target Milestone: --- → mozilla54
Assignee | ||
Comment 10•8 years ago
|
||
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.
Description
•