use enum classes for CSS property enumerated value constants (people interested in this bug: see comment 10 and comments 18-21)

NEW
Unassigned

Status

()

defect
P3
normal
3 years ago
28 days ago

People

(Reporter: dbaron, Unassigned, Mentored)

Tracking

(Depends on 6 bugs, Blocks 1 bug, {good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][see comment 10])

Attachments

(1 attachment)

We should continue the work started in bug 1223653 and use enum classes for CSS property constants, for improved type safety.
Depends on: 1224918
I can continue working on bug 1224918 and submit a patchset to make things work better in nsRuleNode. But I haven't figured out a good way to convert keyword tables to be generic. We can probably open another bug for that.

Ideally, I think keyword tables should be a class like:
> template<typename T>
> class KTable
> {
>   nsCSSKeyword mKeywords[];
>   T mValues[];
> };
So that value type (usually just 8bit) does not need to align with nsCSSKeyword (which is 16bit), and searching through either values or keywords would have a better locality thus improved performance (though the benefit would likely be marginal.)

But I cannot find out a way to accept keyword-value pairs and convert them to the form above in compile time. (Rust macro can really achieve this, but not C++.) It could be done via using an addition script phase, but I'm not sure whether it's worth...

Probably we should just make KTableEntry generic and simply ignore the alignment problem given it preexists... (I forget what was the problem with converting KTableEntry to be generic... I think there was some...)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #1)
> Probably we should just make KTableEntry generic and simply ignore the
> alignment problem given it preexists... (I forget what was the problem with
> converting KTableEntry to be generic... I think there was some...)

Oh, I see what the problem would be in bug 1224918 comment 3 :)
Depends on: 1277773
idorn, what do you think about this, to make the nsCSSProps.cpp entries easier and safer to initialize?  (Relies on mozilla::UnderlyingType, which I have a patch for too.)
Attachment #8773707 - Flags: feedback?(xidorn+moz)
Comment on attachment 8773707 [details] [diff] [review]
allow KTableEntry to be initialized with enums

Review of attachment 8773707 [details] [diff] [review]:
-----------------------------------------------------------------

OK, this is probably the easiest way to go.

FWIW, the trait can be simplified to something like
> template<typename Enum, typename Storage>
> struct IsEnumFittingWithin
> {
>   static constexpr bool value = std::numeric_limits<Storage>::digits >=
>     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> };
Attachment #8773707 - Flags: feedback?(xidorn+moz) → feedback+
Or move the bool expression into the template argument of IntegralConstant...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> FWIW, the trait can be simplified to something like
> > template<typename Enum, typename Storage>
> > struct IsEnumFittingWithin
> > {
> >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > };

I guess this would allow |enum A : uint8_t| to fit within an int8_t storage, though I'm not sure we want to allow that.  Maybe checking min() / max() values would be a more obvious way to check than what I did in my patch.
(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > FWIW, the trait can be simplified to something like
> > > template<typename Enum, typename Storage>
> > > struct IsEnumFittingWithin
> > > {
> > >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> > >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > > };
> 
> I guess this would allow |enum A : uint8_t| to fit within an int8_t storage,
> though I'm not sure we want to allow that.  Maybe checking min() / max()
> values would be a more obvious way to check than what I did in my patch.

It shouldn't: http://en.cppreference.com/w/cpp/types/numeric_limits/digits

Checking min() / max() is also ok as far as it is only used with integers. Using numeric_limits::digits would additionally allow extending that to floating-points, which isn't very useful here, though.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> (In reply to Cameron McCormack (:heycam) from comment #6)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > > FWIW, the trait can be simplified to something like
> > > > template<typename Enum, typename Storage>
> > > > struct IsEnumFittingWithin
> > > > {
> > > >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> > > >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > > > };
> > 
> > I guess this would allow |enum A : uint8_t| to fit within an int8_t storage,
> > though I'm not sure we want to allow that.  Maybe checking min() / max()
> > values would be a more obvious way to check than what I did in my patch.
> 
> It shouldn't: http://en.cppreference.com/w/cpp/types/numeric_limits/digits

Oops, meant to write that around the other way: allowing |enum A : int8_t| to fit within uint8_t, since digits() for the former will be 7 and 8 for the latter.
Hmmm, yeah, you're right. I guess we don't want to allow that. And checking min() / max() and integrality is probably the most precise way. And that way, I'd suggest using static_assert() inside the struct body to check whether the types are enum and integer, rather than relying on error after SFINAE. The error from the later is much more confusing than a straightforward static_assert().
Marking as easy bug, willing to mentor.

This bug involves taking the `#define`d constants from nsStyleConsts.h (https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#117), and making them into enum classes instead.

You can find example patches for this in https://bugzilla.mozilla.org/show_bug.cgi?id=1288383. A relatively straightforward one is https://reviewboard.mozilla.org/r/66042/diff/11#index_header

Some things to watch out for:

 - In many places you may have to use `mozilla::StyleFooBar` instead of `StyleFooBar`. Please omit the `mozilla::` if there is a `using namespace mozilla` or `namespace mozilla{` at the top of the file.

 - In case one of the enum variants is named None, call it None_ instead

 - While the linked patches cast the enums to `uint8_t` in nsCSSProps.cpp, that is no longer necessary.

 - Please don't upload patches to this bug itself, create a new bug (if you want you can link the patch here and I'll create an assigned bug for you). Try not to change too many enums at once, and preferably split it out so that one enum is changed per patch.

Please let me know if you have any questions!
Mentor: manishearth
Keywords: good-first-bug
Whiteboard: [lang=c++][see comment 10]
Depends on: 1291667
Depends on: 1293604
Depends on: 1297982
In case anyone wanna accelerate their "constants-replacing" process, I've written a python script for doing it - https://github.com/Wafflespeanut/scripts/blob/gh-pages/python/ns_style_consts.py (it also requires a nearby script - https://github.com/Wafflespeanut/scripts/blob/gh-pages/python/helpers.py for executing a subprocess)

I agree that it's unnecessarily complicated (I kept playing with it in my free time, just to make the problem more interesting than how it was previously). Now, it works for most of the basic constants in `nsStyleConsts.h`, except a few, which have values spanned over multiple lines. `hg grep` is quite slow in my machine, so I added DXR support (with the help of `dryscrape` module), but that's optional.

I do something like this.

`python ns_style_consts.py ns style will`

It matches over the lines containing `NS_STYLE_WILL_CHANGE_*`, and shows all about how things are gonna change. I review it once and rerun the command with a `-w` flag (and only then it actually makes changes to files). 

The script only matches the "directly visible constants" - it's not at all intelligent to know anything about the types in individual struct fields, function returns, variable comparisons, or our `DEFINE_ENUM_CLASS_SETTER` workaround. That said, it still saves a lot of copy-paste work :)

Updated

3 years ago
Depends on: 1312173
>  - In case one of the enum variants is named None, call it None_ instead

The above workaround in comment 10 is no longer needed (fixed by bug 1300337). We should just use None.

Updated

3 years ago
Depends on: 1313565
Depends on: 1325006

Comment 13

2 years ago
Is this being worked on? Can I take this up? I am new here. So I may need some help.
Hi. This is a tracking bug. It can't be finished straightaway. We usually pick a set of constants (sharing the same prefix) and change them to an enum class (see the other bugs that have addressed this above). You can do the same. Feel free to ping us if you have any questions :)

Updated

2 years ago
Depends on: 1325940
Depends on: 1326125
Depends on: 1339765

Comment 15

2 years ago
Do we want to update all constants in nsStyleConsts.h? I was looking at a few this morning, for example NS_STYLE_AZIMUTH/ELEVATION/PITCH/SPEAK (lines 233 - 276) and they don't appear to occur in any files other than some pub enums in servo. On the other hand, something like NS_STYLE_IMAGELAYER_* is used more extensively (193 hits on DXR vs 36).
Priority: -- → P3

Comment 16

2 years ago
hi @dbaron , i would like to work on this bug. Can you give me some pointers on how to start.

Comment 17

2 years ago
hi Manish , i would like to get working on this bug. Can you give me some pointers on how to start.
Flags: needinfo?(manishearth)

Comment 18

2 years ago
(In reply to akriti verma from comment #17)
> hi Manish , i would like to get working on this bug. Can you give me some
> pointers on how to start.

I would start by looking through layout/style/nsStyleConsts.h [1] and picking a set of #define'd constants you would like to change to enum classes. Once you've settled on a set of constants, make the change throughout the codebase. Using a tool like grep or DXR [2] can be helpful in finding the locations of the constants you would like to update.

You can use the enum classes that are already there as a template.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#117
[2] https://dxr.mozilla.org/mozilla-central/source/
And this bug should be a metabug -- each actual conversion should be filed as a separate bug.  See the "Depends on:" list above for examples.
Flags: needinfo?(manishearth)

Comment 20

2 years ago
Do we need to change all the #define constants, because there will be many enum classes then , under different names which may affect other dependencies.
Flags: needinfo?(manishearth)
Change one set of #define constants into one enum, and fix the fallout, yes. Don't fix all the #defines at once, only one group of them.

See bug 1369448 for an example.
Flags: needinfo?(manishearth)
Depends on: 1389862
Depends on: 1390400

Updated

2 years ago
Depends on: 1403034
Depends on: 1404237

Comment 22

a year ago
I want to resolve to this issue.Please assign me
Flags: needinfo?(manishearth)
(In reply to Ekta Tiwari from comment #22)
> I want to resolve to this issue.Please assign me

See comment 18 and comment 19 for how to get started on a piece of this.  And see the closed bugs listed in the "Depends on:" field at the top of this bug for examples of patches for other already-fixed pieces, if that helps.
Flags: needinfo?(manishearth)

Comment 24

a year ago
Hi, 
I would like to work on this bug. 
Can you assign this bug to me?
Depends on: 1460439

Comment 25

10 months ago
Hello,
I would like to work on this bug.
(In reply to Sidharth from comment #24)
> Hi, 
> I would like to work on this bug. 
> Can you assign this bug to me?

(In reply to Devika Sugathan from comment #25)
> Hello,
> I would like to work on this bug.

Thanks! See comment 18 / comment 19 for how to get started.
Summary: use enum classes for CSS property enumerated value constants → use enum classes for CSS property enumerated value constants (people interested in this bug: see comment 10 and comments 18-21)

Comment 27

8 months ago
What testing should be performed when making these changes apart from ensuring Firefox still builds? Are there any specific unit tests, integration tests, etc.?
(In reply to Ian Gralinski from comment #27)
> What testing should be performed when making these changes apart from
> ensuring Firefox still builds? Are there any specific unit tests,
> integration tests, etc.?

Building is generally enough for these changes.

Comment 29

8 months ago
I have created a new bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1487042) for part of this change. Can I add this to the list of bugs this Depends On?
Depends on: 1487042

Comment 30

6 months ago
Created bug 1500439 to track NS_STYLE_ANIMATION_PLAY_STATE conversion.

Updated

5 months ago
Depends on: 1509664

Comment 31

5 months ago
Hi,

I'm looking for a first bug to work - if possible could I be assigned to this (or some subtask of this if it has be redefined as a meta-bug since comments 18/19)

Thanks!

Comment 32

4 months ago
Hi,
I am new to open source and don't know how to get started.
Please help me out here?

Comment 33

4 months ago
Hello,

I am new to open source (this is my first bug fixing for mozilla) and took sometime to go through this bug. I made changes to the code but it's failing to compile. Can I get some help regarding this. I am working on #define NS_STYLE_COLOR_ADJUST tag.
 

Thanks!
Hi! Please file a new bug, and attach your patch there, I'm happy to take a look and help out with the build issue :)
Depends on: 1516221

Updated

2 months ago
Depends on: 1526447

Comment 35

2 months ago

Hi, I am working on NS_STYLE_FLEX_DIRECTION. I have made changes to the code, but the build fails. Can someone please help me with this?

(In reply to asfiyab95 from comment #35)

Hi, I am working on NS_STYLE_FLEX_DIRECTION. I have made changes to the code, but the build fails. Can someone please help me with this?

Can you file a new bug for that and post there the build failure and the patch? I'm happy to take a look :)

Depends on: 1528940

Can I work on this one?

You need to log in before you can comment on or make changes to this bug.