1825386 - Convert LogicalAxis to enum class in WritingMode.h

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Oliver Medhurst [:canadahonk]]

Split-off bug for the base Logical* enums (LogicalAxis, LogicalEdge, LogicalSide, LogicalCorner). Probably best to leave LogicalSideBits for later.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Let's morph this bug into convert LogicalAxis to an enum class. https://searchfox.org/mozilla-central/rev/5f0a7ca8968ac5cef8846e1d970ef178b8b76dcc/layout/generic/WritingModes.h#49-52

For other enum class conversion, I've filed individual bugs blocking bug 1610666 for them.

See https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html#variable-prefixes for the coding style for enum class.

Comment 2

[siddharthaswarnkar]

Hello, it's my first time trying to contribute. I saw a similar bug issue before, but I think it's already being worked on. I think I can fix this bug. Can this issue be assigned to me ?

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Yeah, other similar bugs are good references. If you haven't had a develop environment for Firefox , you can start from https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html. Once you attach a patch to this bug, you'll be assigned to this bug automatically by a bot.

Comment 4

[siddharthaswarnkar]

I have converted all to enum class and also followed the coding convention, but now that I am building, I am getting this error, I tried type casting as uint8_t(aAxis) but then it throws an unidentified identifier error.

0:14.59 F:/mozilla-source/mozilla-unified/obj-x86_64-pc-windows-msvc/dist/include/mozilla/WritingModes.h(346,56): error: use of undeclared identifier 'uint_8'
0:14.59 346 | return mozilla::PhysicalAxis((aWritingModeValue ^ (uint_8(aAxis))) & 0x1);
0:14.59 | ^
0:14.60 1 error generated.
^

But type casting works with this function,

inline LogicalSide MakeLogicalSide(LogicalAxis aAxis, LogicalEdge aEdge) {
  return LogicalSide((uint8_t(aAxis) << 1) | aEdge);
}

How do I fix this ?

Comment 5

[siddharthaswarnkar]

Really sorry, didn't see my typo there

Comment 6

[siddharthaswarnkar]

Attachment: Bug 1825386 - Converted LogicalAxis to enum class and type casted where necessary. r=TYLin

LogicalAxis is one of the Logical* enums. Converting it from enum to
enum class increases type safety.

Comment 7

[siddharthaswarnkar]

I have one last query, should I have been more descriptive in the commit ? Because I also had to update the function return types and at places had to type cast the enum class to uint8_t to match the pre-existing code.

Comment 8

[siddharthaswarnkar]

*not function return types, only type casting

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 7: yeah, it is always welcome to be more descriptive in the commit message, but in this case, type casting is somewhat expected since an enum class cannot convert to an integer automatically. I'll leave the judgement to you whether you'd want to update the commit message.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dea770591ed3
Converted LogicalAxis to enum class and type casted where necessary. r=TYLin

Comment 11

[Cristian Tuns]

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