using enum classes for enumerated CSS property values in nsStyleConsts.h

[L. David Baron]

Jesse suggested using enum classes for the values of enumerated CSS
properties, and I tend to agree that this is a good idea. It gives
both stricter typechecking than the current uint8_t (etc.) values,
and better warnings for switch() statements omitting values.

I decided to try this out for box-sizing to see what it would look
like. Patches are linked from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1223653#c1

Interestingly, the typechecking actually caught a real bug, where we
were passing an nscoord (size inside the box-sizing) to a function
that expected a uint8_t (enumerated value of box-sizing property),
because we actually intended to call a different function.

But I'm mainly interested in feedback on:

1. Naming
=========

Do the names I chose seem reasonable? I replaced:
-#define NS_STYLE_BOX_SIZING_CONTENT 0
-#define NS_STYLE_BOX_SIZING_PADDING 1
-#define NS_STYLE_BOX_SIZING_BORDER 2
with:
+namespace mozilla {
+
+enum class StyleBoxSizing : uint8_t {
+ Content,
+ Padding,
+ Border
+};
+
+} // namespace mozilla
so that most caller changes look like:
- case NS_STYLE_BOX_SIZING_BORDER:
+ case StyleBoxSizing::Border:
or like:
if (parent &&
- parent->StylePosition()->mBoxSizing != NS_STYLE_BOX_SIZING_BORDER) {
+ parent->StylePosition()->mBoxSizing != StyleBoxSizing::Border) {

2. Casts
========

Does anybody see a good way to eliminate the casts I needed to use
in the main patch (described in the patch header)?

3. General
==========

Do others agree this is a good idea? And are people ok with it
being done gradually?

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Xidorn Quan]

The names make sense to me.

> 2. Casts
> ========
>
> Does anybody see a good way to eliminate the casts I needed to use
> in the main patch (described in the patch header)?

We can probably make them templates?

> 3. General
> ==========
>
> Do others agree this is a good idea? And are people ok with it
> being done gradually?

Yeah, I think this is a great idea. We should benefit more from the type system.

Given you are doing this, I'd like to try this approach in my new
patches as well.

One more thing may need to be done before more conversion comes: we
need a typesafe way to declare bitmasks which we are using for several
properties. I filed bug 1217241 for something similiar several weeks
ago.

- Xidorn

[Cameron McCormack]

L. David Baron:

> 1. Naming
> =========
>
> Do the names I chose seem reasonable? I replaced:
> -#define NS_STYLE_BOX_SIZING_CONTENT 0
> -#define NS_STYLE_BOX_SIZING_PADDING 1
> -#define NS_STYLE_BOX_SIZING_BORDER 2
> with:
> +namespace mozilla {
> +
> +enum class StyleBoxSizing : uint8_t {
> + Content,
> + Padding,
> + Border
> +};
> +
> +} // namespace mozilla
> so that most caller changes look like:
> - case NS_STYLE_BOX_SIZING_BORDER:
> + case StyleBoxSizing::Border:
> or like:
> if (parent &&
> - parent->StylePosition()->mBoxSizing != NS_STYLE_BOX_SIZING_BORDER) {
> + parent->StylePosition()->mBoxSizing != StyleBoxSizing::Border) {

Those names look good to me.

> 2. Casts
> ========
>
> Does anybody see a good way to eliminate the casts I needed to use
> in the main patch (described in the patch header)?

For the KTables, you could define KTableValue as a class that has
implicit conversion constructors taking nsCSSKeyword and StyleBoxSizing
(and all the other new ones when they’re added).

For the casts in SetDiscrete, no, although I don’t think it’s much of a
problem. (I suppose you could do something where you have a template
wrapper class for nsCSSValue that identifies the particular enum that an
eCSSUnit_Enumerated in the wrapped nsCSSValue represents, and then have
nsRuleData::ValueForXXX return types like
nsCSSValueWrapper<StyleBoxSizing>, but it seems over the top.)

> 3. General
> ==========
>
> Do others agree this is a good idea? And are people ok with it
> being done gradually?

Yes, I think it’s great.

--
Cameron McCormack ≝ http://mcc.id.au/

[L. David Baron]

On Wednesday 2015-11-11 17:29 +1100, Xidorn Quan wrote:
> On Wed, Nov 11, 2015 at 5:07 PM, L. David Baron <dba...@dbaron.org> wrote:

> > 2. Casts
> > ========
> >
> > Does anybody see a good way to eliminate the casts I needed to use
> > in the main patch (described in the patch header)?
>

> We can probably make them templates?

How?

> One more thing may need to be done before more conversion comes: we
> need a typesafe way to declare bitmasks which we are using for several
> properties. I filed bug 1217241 for something similiar several weeks
> ago.

I think mfbt/TypedEnumBits.h already does this.

[Xidorn Quan]

On Wed, Nov 11, 2015 at 5:40 PM, L. David Baron <dba...@dbaron.org> wrote:
> On Wednesday 2015-11-11 17:29 +1100, Xidorn Quan wrote:
>> On Wed, Nov 11, 2015 at 5:07 PM, L. David Baron <dba...@dbaron.org> wrote:
>> > 2. Casts
>> > ========
>> >
>> > Does anybody see a good way to eliminate the casts I needed to use
>> > in the main patch (described in the patch header)?
>>
>> We can probably make them templates?
>
> How?

Ahh, I forgot that KTableValue is a single value which accepts both
keywords and computed constants.

We may want to replace KTableValue[2] with a struct template like

template<typename T>
struct KTableItem
{
nsCSSKeyword keyword;
T value;
};

and convert all functions which accept KTables to templates.

It would be a much larger change, though.

- Xidorn

[L. David Baron]

I'm in favor of making it a struct -- but if it becomes a template,
won't the functions that accept it have to become templates,
generating multiple copies of the code where we currently have only
a single copy?

[Cameron McCormack]

L. David Baron:

> I'm in favor of making it a struct -- but if it becomes a template,
> won't the functions that accept it have to become templates,
> generating multiple copies of the code where we currently have only
> a single copy?

You’d hope that the compiler would fold together all the versions of the
template that took the same sized enum. (You could always have a small
inline function that casts to uint8_t and have a common implementation
that works on the untyped values.)

[Xidorn Quan]

I hope compilers do that for us, but I have no idea whether it is that case.

Having a small inline template function for each of those functions
sounds painful to me. If there are not too many such functions, it
could probably be fine, though.

- Xidorn

[Daniel Holbert]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 11/10/2015 10:40 PM, L. David Baron wrote:
>> One more thing may need to be done before more conversion comes:
>> we need a typesafe way to declare bitmasks which we are using for
>> several properties. I filed bug 1217241 for something similiar
>> several weeks ago.
>
> I think mfbt/TypedEnumBits.h already does this.

As a heads-up, there's one slightly-awkward thing I've run into with
TypedEnumBits.h's "MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS" magic:
there's no implicit conversion to bool, when you try to put the result
of a bitmask operation into a variable or function-arg.

Specifically -- suppose I have "MyMaskType foo", and I want to test if
it has the bit MyMaskType::SomeBit set. The compiler will happily
accept this:
if (foo | MyMaskType::SomeBit)

But, the compiler rejects this:
bool isBitSet = foo | MyMaskType::SomeBit;

...and more importantly, this:
SomeFunctionWhichTakesABoolArg(foo | MyMaskType::SomeBit);

The error looks like this, in clang:
error: no viable conversion from
'mozilla::CastableTypedEnumResult<MyMaskType>' to 'bool'

The compiler forces me to use "!!" or "bool(...)" to make the boolean
conversion explicit in these latter two cases, which makes these
type-safe masks a bit clumsier to work with.

~Daniel

P.S. (I can work around this by adding an "operator bool" to the
CastableTypedEnumResult type-definition in mfbt/TypedEnumBits.h. Maybe
that's something we should do.)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWQuyOAAoJEOxXkPfRs1MyqcAP/0xZG0iIpkiO5FTPdvsl+ZUM
23pS/sG1BiNOJ7NudIeRX0hGfgw47QUQNpZh/aKz+ZUengCu1n/Y+b+dxV2uIU/1
ZYz+tsHrd0rco/YhxS6HjlSr/Pl8d7jq6In0aKv7/E9BDyi6UrlxHq4t0S4VNSxT
m1Y+q3vm+vQtrTvd7fznrrL0NQ2WJDsVcKEgZxO13sodeUsuPbDe5uXS6hJTnu6N
uVmsDrP61dMw14s4CUTuli4AmYs2+jrSfrDBHl72kGszY7louMDqlvMsjJCFwbfa
DlALtP2/8oM67+WEPr/YPulDSCrXT+8VeBCaBLi58noCEesP9sf+0e8Yxt2GX3LF
yRqzqH/cvv2v4/6M3Hvd5W5RMDDVhuxfy8jKl4fTMDp5JCa8ponpE1wVFbgF7gF7
pzq1dnrkNEsEpP+IBhy0B6tBApeu1v5jqwPyzv0+D7QiHIm2tgSFDYEHXZ+GmJLn
oyGtE2MGjBuMU+AiU0QSEvxWg9tQ3d92oYrUdxY5qGp+cl3Wujdy9H0188ZpLjTl
425vA8OSQFwxrLmxJDsK5Jg5+UgjwePRzSkOfE8JvYHk/hvVXkTo0/uC9/2W+GL1
gRYjTAspRYu+iwd4nNcT1tVwP2/1yGPaAr/0M/175BM76DGK8EJOsq2F6ICkZfKc
f2Ev3jLL4cKFH4auExR5
=Rtti
-----END PGP SIGNATURE-----

[Xidorn Quan]

On Wed, Nov 11, 2015 at 6:21 PM, Daniel Holbert <dhol...@mozilla.com> wrote:
>
> P.S. (I can work around this by adding an "operator bool" to the
> CastableTypedEnumResult type-definition in mfbt/TypedEnumBits.h. Maybe
> that's something we should do.)

We should just add "explicit operator bool() const;" to
CastableTypedEnumResult, which we are allowed to use since VS2012
support was dropped.

- Xidorn

[Xidorn Quan]

On Wed, Nov 11, 2015 at 6:03 PM, Xidorn Quan <quanx...@gmail.com> wrote:
> On Wed, Nov 11, 2015 at 5:59 PM, Cameron McCormack <c...@mcc.id.au> wrote:
>> L. David Baron:
>>> I'm in favor of making it a struct -- but if it becomes a template,
>>> won't the functions that accept it have to become templates,
>>> generating multiple copies of the code where we currently have only
>>> a single copy?
>>
>> You’d hope that the compiler would fold together all the versions of the
>> template that took the same sized enum. (You could always have a small
>> inline function that casts to uint8_t and have a common implementation
>> that works on the untyped values.)
>
> I hope compilers do that for us, but I have no idea whether it is that case.

Confirmed from a gcc developer that compilers are unlikely to do that
for us because that could cause problems in several cases (e.g. if
there is a static variable declared inside the function body).

So if we want to do this, we would have to add an inline template
function for each of those functions to do the type conversion. It
could be painful, but probably not too much.

- Xidorn

[Daniel Holbert]

Cool -- though, are you sure we'd want "explicit"?

The point of my email was that *implicit* conversion to bool is
convenient & desirable (IMO) for these bitmask types, particularly when
passing them as a boolean function-argument.

I don't see too much danger from bool-conversion here, and I'd much
rather type:
SomeFunc(foo | MyMaskType::SomeBit);

...rather than having to use explicit conversion like:
SomeFunc(static_cast<bool>(foo | MyMaskType::SomeBit)));

If we use the "explicit" keyword, then we'd have to resort to the second
example here. (or we'd have to use "!!", or we'd have to rewrite
SomeFunc to stop taking a bool)

~Daniel