1289710 - allow KTableEntry objects to be initialized with enum values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

From bug 1277133 comment 3.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1289710 - Allow KTableEntry objects to be initialized with enum values of appropriate size.

Review commit: https://reviewboard.mozilla.org/r/67400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67400/

Comment 2

[Cameron McCormack (:heycam)]

Building: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a01ecc81e67b
Failing when using an inappropriately sized enum: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3911de9e6b&selectedJob=24592340

Comment 3

[Cameron McCormack (:heycam)]

I notice that, for example, nsCSSProps::ValueToKeywordEnum takes an int32_t, even though all valid keywords must fit within an int16_t.  ValueToKeywordEnum then asserts that the value will fit.  Should we do the same for the KTableEntry constructor that takes the integer type?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8775080 [details]
Bug 1289710 - Allow KTableEntry objects to be initialized with enum values of appropriate size.

https://reviewboard.mozilla.org/r/67400/#review64434

Looks good. Thanks.

::: layout/style/nsCSSProps.h:359
(Diff revision 1)
> +struct IsEnumFittingWithin
> +  : std::conditional<std::is_enum<T>::value,
> +                     detail::IsEnumFittingWithinHelper<T, Storage>,
> +                     std::false_type>::type

Given you call it "IsEnumFittingWithin", how about just static_assert that T is an enum, and get rid of the std::conditional wrap and just make this struct inherit IsIntegralFittingWithin directly?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #3)
> I notice that, for example, nsCSSProps::ValueToKeywordEnum takes an int32_t,
> even though all valid keywords must fit within an int16_t. 
> ValueToKeywordEnum then asserts that the value will fit.  Should we do the
> same for the KTableEntry constructor that takes the integer type?

I don't think that matters. It doesn't seem to me the assertion in nsCSSProps::ValueToKeywordEnum makes any sense actually.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Given you call it "IsEnumFittingWithin", how about just static_assert that T
> is an enum, and get rid of the std::conditional wrap and just make this
> struct inherit IsIntegralFittingWithin directly?

I can do that, although std::underlying_type seems to cause compilation failures that are no good for SFINAE when passed a non-enum type.  But since I need to use std::enable_if<std::is_enum<...>> on the constructor anyway, that isn't a problem.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8775080 [details]
Bug 1289710 - Allow KTableEntry objects to be initialized with enum values of appropriate size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67400/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76a64989d218

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e41e5886c0f
Allow KTableEntry objects to be initialized with enum values of appropriate size. r=xidorn

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e41e5886c0f