Closed Bug 1266391 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 1 obsolete file)

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.)
See Also: → 1266341
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: 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+
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.
Attachment #8743835 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/1b2c3cc8c06f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.