Closed Bug 1289710 Opened 3 years ago Closed 3 years ago

allow KTableEntry objects to be initialized with enum values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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?
Attachment #8775080 - Flags: review?(xidorn+moz) → review+
(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.
(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 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/
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e41e5886c0f
Allow KTableEntry objects to be initialized with enum values of appropriate size. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/5e41e5886c0f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.