Use an enum class for script codes, instead of just passing integers around

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
mozilla::unicode::GetScriptCode currently returns a uint8_t, and we generally pass script codes around (mostly in gfx code) as int32_t. This is error-prone, as we can easily use an invalid script code without noticing; see bug 1266341.

So what I propose is to declare our MOZ_SCRIPT_* values as an enum class mozilla::unicode::Script, and then the compiler can check that we're specifying and passing the right codes around everywhere.

(There are a few places where we work directly with the numeric values of the script codes; where this is needed, we'll add explicit casts to let us work with them as integers. Most of the time, though, they're simply tokens that we look up, pass around and compare for equality.)
Assignee

Updated

3 years ago
See Also: → 1266341
Assignee

Comment 1

3 years ago
Sorry about the big patch, but it really needs to happen all at once. Most of the changes are trivial, anyhow. (This builds & runs for me locally, but I haven't built on all platforms yet, so it's possible there will be an update if tryserver says I've missed something.)
Attachment #8743835 - Flags: review?(masayuki)
Assignee

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8743835 [details] [diff] [review]
Introduce an enum class mozilla::unicode::Script, and use this instead of bare integers to specify script codes for better type checking

I just checked this patch briefly because I'm not familiar with current gfx. So, I don't check there are more code which needs to replace int32_t to Script.

Some points:
1. I assume that you modified the enum definition in nsUnicodeScriptCodes.h mechanically. So, I assume that there is no mistake of naming and specifying the number.

2. I like to use static_cast for casting int32_t <-> enum class. But this is pretty big patch. So, you should land this ASAP. If you agree with this, I hope you will create follow up patch, but up to you if it's necessary. (*_cast is easier to search such dangerous point)

3. In a couple of places, you cast computed int value to Script. If I were you, I'd create a method to convert int32_t to Script with validation. But I'm not sure if it causes increasing binary size or making damage to the performance due to gfx. So, I'm not sure if doing that is better.
Attachment #8743835 - Flags: review?(masayuki) → review+
Assignee

Comment 3

3 years ago
Updated to fix a build failure on android, and added some static_cast goodness as suggested; carrying over r=masayuki. I'll run this through try before attempting to land... https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b52e76ec15.
Assignee

Updated

3 years ago
Attachment #8743835 - Attachment is obsolete: true
Assignee

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2c3cc8c06f7e257a8040c04086a99889356b2d
Bug 1266391 - Introduce an enum class mozilla::unicode::Script, and use this instead of bare integers to specify script codes for better type checking. r=masayuki

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b2c3cc8c06f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.