1541095 - Try to use typed enums to avoid need for static_casts in WritingModes.h

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Daniel Holbert [:dholbert]]

In bug 1540357 ( https://phabricator.services.mozilla.com/D25499 ), jgilbert's addressing some build warnings in WritingModes.h by using static_cast expressions.

Stuff like this, where wm needs to have type uint8_t, mWritingMode is a uint8_t, and eOrientationMask is an enum which means it's really int-flavored:

    const auto wm = static_cast<uint8_t>(mWritingMode & eOrientationMask);

If we can make eOrientationMask be uint8_t flavored, then we won't need the static_cast. Filing this bug on seeing if we can do that, to remove the gross static_cast expressions.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

eOrientationMask has been removed in Bug 1578147, and there's no more static_cast. Is there anything left to do in this bug?

Comment 2

[Daniel Holbert [:dholbert]]

The static_cast that I was quoting in comment 0 was indeed removed, from the impl of mozilla::PhysicalAxis PhysicalAxis(LogicalAxis aAxis) const {.

But the other one that we added to WritingModes.h in https://phabricator.services.mozilla.com/D25499 (in PhysicalSide) still remains:

https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/layout/generic/WritingModes.h#445,450-452

But I don't think we actually need that one anymore? I seem to be able to build just fine locally, at least, when I remove that static_cast and change the auto wm var there to have type uint8_t. And indeed, it looks like the ***.bits masks there have type uint8_t, which is presumably why this is fine:

(The current contents of https://searchfox.org/mozilla-central/source/__GENERATED__/layout/style/ServoStyleConsts.h#11407 are:

struct StyleWritingMode {
  uint8_t bits;

)

Comment 3

[Daniel Holbert [:dholbert]]

I'll make that change and see if Try likes it... (not sure if I have the right configuration locally to hit the Werror failure that bug 1540357 set up, so I'll let Try be the judge)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1541095: Remove an unnecessary static_cast in WritingModes.h. r?TYLin

The variables involved in the bitwise arithmetic are all of type uint8_t, so
we end up with that type automatically (no need to bother with a static_cast).

Comment 5

[Daniel Holbert [:dholbert]]

Here's a failed try run with the initial patch here (dropping static_cast<uint8_t>(...) wrapper and making no other changes:)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=add996864b30e772d3f1248b26e3785ad2c552c5&selectedJob=287801119

...and here's a successful try run where I also changed the variable type to uint8_t (instead of auto):
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=287815967&revision=c331af46a22518690e91f570b57a49c98b3d20f8
(This successful one is the patch that's on phabricator now.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1f26a8866c1
Remove an unnecessary static_cast in WritingModes.h. r=TYLin

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f1f26a8866c1