Bug 224981 - Refactor ValueRange from enum to enum class
Summary: Refactor ValueRange from enum to enum class
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
Keywords: InRadar
Depends on:
Reported: 2021-04-23 09:08 PDT by Tyler Wilcock
Modified: 2021-04-24 10:11 PDT (History)
13 users (show)

See Also:

Patch (78.27 KB, patch)
2021-04-23 09:19 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2021-04-23 09:08:56 PDT
ValueRange looks like this:

enum ValueRange {

It should be an `enum class : uint8_t`.  This would make it smaller (one byte versus four) and harder to misuse.
Comment 1 Tyler Wilcock 2021-04-23 09:19:42 PDT
Created attachment 426915 [details]
Comment 2 Tyler Wilcock 2021-04-23 17:50:57 PDT
Darin, here's one follow-up from https://bugs.webkit.org/show_bug.cgi?id=224718#c24 (parsing @counter-style descriptors).
Comment 3 EWS 2021-04-24 09:02:16 PDT
Committed r276550 (236990@main): <https://commits.webkit.org/236990@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426915 [details].
Comment 4 Radar WebKit Bug Importer 2021-04-24 09:03:14 PDT
Comment 5 Darin Adler 2021-04-24 10:11:58 PDT
Comment on attachment 426915 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=426915&action=review

> Source/WebCore/ChangeLog:23
> +        making it smaller (one byte versus four) and harder to misuse (no
> +        auto-coercion to number types).

Seems like a great change.

For future reference, adding the underlying type of uint8_t is independent of changing from enum to enum class. Both are good ideas.

I believe that for two-value enumerations, bool is an even better underlying type than uint8_t, and on the modern compilers we are compiling with both will use a single byte for storage.