Closed Bug 1224918 Opened 4 years ago Closed 3 years ago

find a better solution for casting away enum class safety for enumerated css property values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- affected
firefox49 --- fixed

People

(Reporter: dbaron, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

In bug 1223653 patch 2, I have to add a few casts to cast away the safety we get from enum classes, as part of the process of introducing the use of enum classes for enumerated values of CSS properties.

However, I'd like to find a better solution for this.  bug 1224464 is the first piece of that, but I'm not quite sure how to continue.  There's some discussion in https://groups.google.com/d/topic/mozilla.dev.tech.layout/eYJkMF84rsE/discussion .
xidorn has proposals for how to do this in bug 1223653.
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #1)
> xidorn has proposals for how to do this in bug 1223653.

Actually, those comments were, I think, about a different problem, which is how to avoid changing the value passed for the ignored parameters to SetDiscrete, rather than avoiding the need for the casts inside of SetDiscrete.
That also avoids casts inside of SetDiscrete, no?

Reading the patch in bug 1223653, it seems to me the hardest part of this bug would be fixing the casts in nsCSSProps.cpp. Making KTableEntry a template doesn't work perfectly, because we use a sentinel entry "{ eCSSKeyword_Unknown, -1 }" to mark the end of tables, and if we keep doing so, we would have to make all enums base on signed integer, and cast the -1 to corresponding enum classes.

I feel like it would be better to switch to checking the count instead of the sentinel entry. But the count number should be computed automatically. It seems to be challenging to keep the syntax simple while doing so, especially in compile time.

I do not have particually good idea here. If we have the power of macros in Rust, it would be a fairly simple task :)
There would also be an issue for kKeywordTableTable and any use of it.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> That also avoids casts inside of SetDiscrete, no?

I don't see how.  The casts I had to add to SetDiscrete were to the results of aValue.GetIntValue().
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #5)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> > That also avoids casts inside of SetDiscrete, no?
> 
> I don't see how.  The casts I had to add to SetDiscrete were to the results
> of aValue.GetIntValue().

Ah, you meant that. Right, that doesn't solve those casts. I somehow ignored that...

For the eCSSUnit_Integer case, I think we could have another helper template function, which asserts when the target type is not convertible from an integer. (Then we no longer need the SETDSC_INTEGER flag.)

For the eCSSUnit_Enumerated case, I don't have particular good idea. There's even no automatic way in runtime to ensure that the value is in the proper range, because there's no way in C++ to detect the max/min value of a enum.

One potential solution is to have a helper template function which asserts by default, and we manually instantiate it for each enum class we use, and optionally add runtime check there. (We probably can define a macro to reduce the code need to write for each individual enum class.) The only benefit AFAICS is that we can get rid of the SETDSC_ENUMERATED flag (and thus remove a runtime check which is actually never used) while still keep things safe.
Attached patch proposal patch for SetDiscrete (obsolete) — Splinter Review
This is what I'm currently thinking for SetDiscrete.
Attachment #8691146 - Flags: feedback?(dbaron)
Updated a bit compared with the last patch.
Attachment #8723420 - Flags: feedback?(dbaron)
Attachment #8691146 - Attachment is obsolete: true
Attachment #8691146 - Flags: feedback?(dbaron)
Comment on attachment 8723420 [details] [diff] [review]
proposal patch for SetDiscrete

I guess this is an improvement on what we currently have, although I'd prefer to get compilation errors to indicate failure rather than assertions.

(I wonder if there's a way to get that by making SetDiscrete a template over the flags rather than passing the flags as a parameter?  Although I wonder if that reduces or increases code size...)
Attachment #8723420 - Flags: feedback?(dbaron) → feedback+
I don't think it is possible to use compilation error for that, since aValue.GetUnit() is only known at runtime, which means there would be some form of assertions anyway.
Blocks: 1277133
Attachment #8758561 - Flags: review?(dbaron) → review+
Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

https://reviewboard.mozilla.org/r/56810/#review54158

Maybe it's worth a FIXME comment that it would be nice to make the things that currently MOZ_ASSERT_UNREACHABLE into compilation errors if we can figure out how?
Attachment #8758562 - Flags: review?(dbaron) → review+
Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

https://reviewboard.mozilla.org/r/56812/#review54160

::: layout/style/nsRuleNode.cpp:1342
(Diff revision 1)
>  struct SetDiscreteHelper : Conditional<IsEnum<FieldT>::value,
>                                         SetEnumDiscreteHelper,
>                                         SetIntegerDiscreteHelper<FieldT>>::Type
>  {
>  };
>  

The commit message should say "unsupported units" rather than "unsupport units".

::: layout/style/nsRuleNode.cpp:1354
(Diff revision 1)
> -          typename T1, typename T2, typename T3, typename T4, typename T5>
> +          typename AutoT = unused_t, typename NoneT = unused_t,
> +          typename NormalT = unused_t, typename SysFontT = unused_t>

Are these "= unused_t" here needed or helpful, given that you also have "= Unused" on the values?

::: layout/style/nsRuleNode.cpp
(Diff revision 1)
> -              SETDSC_NONE | SETDSC_AUTO | SETDSC_UNSET_INHERIT,
>                parentText->mTextSizeAdjust,
> -              NS_STYLE_TEXT_SIZE_ADJUST_AUTO, // initial value
> -              NS_STYLE_TEXT_SIZE_ADJUST_AUTO, // auto value
> -              NS_STYLE_TEXT_SIZE_ADJUST_NONE, // none value
> +              /* initial */ NS_STYLE_TEXT_SIZE_ADJUST_AUTO,
> +              /* auto */ NS_STYLE_TEXT_SIZE_ADJUST_AUTO,
> +              /* none */ NS_STYLE_TEXT_SIZE_ADJUST_NONE);
> -              0, 0);

I'm not crazy about having varying numbers of these parameters; I'd somewhat prefer to have either none of the four, or all 4; I think that's easier to read.

So I'd prefer to have an "Unused, Unused" at the end here.

::: layout/style/nsRuleNode.cpp:5745
(Diff revision 1)
>  
>    // contain: none, enum, inherit, initial
>    SetDiscrete(*aRuleData->ValueForContain(), display->mContain, conditions,
> -              SETDSC_ENUMERATED | SETDSC_NONE | SETDSC_UNSET_INITIAL,
> +              SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
>                parentDisplay->mContain,
> -              NS_STYLE_CONTAIN_NONE, 0, NS_STYLE_CONTAIN_NONE, 0, 0);
> +              NS_STYLE_CONTAIN_NONE, Unused, NS_STYLE_CONTAIN_NONE);

and 2 Unuseds here as well

::: layout/style/nsRuleNode.cpp:6008
(Diff revision 1)
> -              SETDSC_ENUMERATED | SETDSC_AUTO | SETDSC_NONE |
> +              SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
> -                SETDSC_UNSET_INITIAL,
>                parentDisplay->mTouchAction,
> -              NS_STYLE_TOUCH_ACTION_AUTO,
> -              NS_STYLE_TOUCH_ACTION_AUTO,
> -              NS_STYLE_TOUCH_ACTION_NONE, 0, 0);
> +              /* initial */ NS_STYLE_TOUCH_ACTION_AUTO,
> +              /* auto */ NS_STYLE_TOUCH_ACTION_AUTO,
> +              /* none */ NS_STYLE_TOUCH_ACTION_NONE);

same here

<p>... in fact, given my comments above, I think I'd prefer to make it impossible to have an intermediate number of those parameters.</p>
<p>I think that should be easy to do by using a function overload (a variant using 4 fewer parameters, calling the other variant with 4 extra Unuseds) instead of default parameters.</p>
<p></p>
<p></p>
<p>Also, after you land this, please double-check that things are ok a few days after you land, and nobody's landed patches that disagree with it.</p>
<p>r=dbaron with that</p>
It's unfortunate that it won't lead to any error when someone uses the old form and pass 0s in. Probably we should rename the function a bit so that we can catch any new use of that function?
I don't have any great naming ideas, though.  Maybe SetUntagged (since it's setting a value that's not part of a tagged union, like SetCoord does)?  Or maybe just SetValue?
I guess SetValue works. Thanks for the suggestions.
Review commit: https://reviewboard.mozilla.org/r/57414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57414/
Attachment #8758561 - Attachment description: MozReview Request: Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes. r?dbaron → Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.
Attachment #8758562 - Attachment description: MozReview Request: Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupport units. r?dbaron → Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.
Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56810/diff/1-2/
Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56812/diff/1-2/
dbaron, could you review the new rename patch (part 0)? I guess you may also want to have a look at the updated part 1, which is changed quite a bit.
Flags: needinfo?(dbaron)
Attachment #8759436 - Flags: review+
Comment on attachment 8759436 [details]
Bug 1224918 part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error.

https://reviewboard.mozilla.org/r/57414/#review54222

Seems like it would have been easier to write this on top of the other two, but I guess it's done now.
... and moving things into SetValueHelper seems fine.
Flags: needinfo?(dbaron)
Comment on attachment 8759436 [details]
Bug 1224918 part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57414/diff/1-2/
Comment on attachment 8758561 [details]
Bug 1224918 part 1 - Make SetDiscrete more type-safe and easy to use with enum classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56810/diff/2-3/
Comment on attachment 8758562 [details]
Bug 1224918 part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56812/diff/2-3/
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8ef068956b
part 0 - Rename SetDiscrete to SetValue so that we can catch any new use of the old pattern with a compilation error. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6d4982db0b
part 1 - Make SetDiscrete more type-safe and easy to use with enum classes. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8b79672ffe
part 2 - Remove SETDCT_{AUTO,NONE,NORMAL,SYSTEM_FONT} and use Unused to indicate unsupported units. r=dbaron
You need to log in before you can comment on or make changes to this bug.