From 8b9b3617a2c67ff49821b4c5ac3df4079c435cc7 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Wed, 7 Feb 2018 14:08:17 +0100 Subject: [PATCH 1/3] =?UTF-8?q?Merge=20all=20keyword=20arms=20in=20Clone?= =?UTF-8?q?=20for=20PropertyDeclaration=20=F0=9F=90=89=F0=9F=90=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We further partition the variants to retrieve all properties that are defined as simple keywords, and we clone them by copying the value as a smaller enum made of only the keyword variants. --- .../style/properties/properties.mako.rs | 59 ++++++++++++++----- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index ee35b7bf4f4..21d585fcb48 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -218,19 +218,20 @@ pub mod animated_properties { "name": property.camel_case, "type": ty, "doc": "`" + property.name + "`", + "keyword": bool(property.keyword) and not property.is_vector, }) groups = {} keyfunc = lambda x: x["type"] sortkeys = {} for ty, group in groupby(sorted(variants, key=keyfunc), keyfunc): - group = [v["name"] for v in group] + group = list(group) groups[ty] = group for v in group: if len(group) == 1: - sortkeys[v] = (1, v, "") + sortkeys[v["name"]] = (not v["keyword"], 1, v["name"], "") else: - sortkeys[v] = (len(group), ty, v) + sortkeys[v["name"]] = (not v["keyword"], len(group), ty, v["name"]) variants.sort(key=lambda x: sortkeys[x["name"]]) # It is extremely important to sort the `data.longhands` array here so @@ -250,21 +251,24 @@ pub mod animated_properties { "name": "CSSWideKeyword", "type": "WideKeywordDeclaration", "doc": "A CSS-wide keyword.", + "keyword": False, }, { "name": "WithVariables", "type": "VariableDeclaration", "doc": "An unparsed declaration.", + "keyword": False, }, { "name": "Custom", "type": "CustomDeclaration", "doc": "A custom property declaration.", + "keyword": False, }, ] for v in extra: variants.append(v) - groups[v["type"]] = [v["name"]] + groups[v["type"]] = [v] %> /// Servo's representation for a property declaration. @@ -287,14 +291,39 @@ impl Clone for PropertyDeclaration { fn clone(&self) -> Self { use self::PropertyDeclaration::*; + <% + [keywords, others] = [list(g) for _, g in groupby(variants, key=lambda x: not x["keyword"])] + %> + match *self { - % for ty, variants in groups.iteritems(): - % if len(variants) == 1: - ${variants[0]}(ref value) => { - ${variants[0]}(value.clone()) + ${" | ".join("{}(..)".format(v["name"]) for v in keywords)} => { + #[derive(Clone, Copy)] + #[repr(u16)] + enum Keywords { + % for v in keywords: + _${v["name"]}(${v["type"]}), + % endfor + } + + unsafe { + let mut out = mem::uninitialized(); + ptr::write( + &mut out as *mut _ as *mut Keywords, + *(self as *const _ as *const Keywords), + ); + out + } + } + % for ty, vs in groupby(others, key=lambda x: x["type"]): + <% + vs = list(vs) + %> + % if len(vs) == 1: + ${vs[0]["name"]}(ref value) => { + ${vs[0]["name"]}(value.clone()) } % else: - ${" | ".join("{}(ref value)".format(v) for v in variants)} => { + ${" | ".join("{}(ref value)".format(v["name"]) for v in vs)} => { unsafe { let mut out = ManuallyDrop::new(mem::uninitialized()); ptr::write( @@ -327,8 +356,8 @@ impl PartialEq for PropertyDeclaration { return false; } match *self { - % for ty, variants in groups.iteritems(): - ${" | ".join("{}(ref this)".format(v) for v in variants)} => { + % for ty, vs in groupby(variants, key=lambda x: x["type"]): + ${" | ".join("{}(ref this)".format(v["name"]) for v in vs)} => { let other_repr = &*(other as *const _ as *const PropertyDeclarationVariantRepr<${ty}>); *this == other_repr.value @@ -346,8 +375,8 @@ impl MallocSizeOf for PropertyDeclaration { use self::PropertyDeclaration::*; match *self { - % for ty, variants in groups.iteritems(): - ${" | ".join("{}(ref value)".format(v) for v in variants)} => { + % for ty, vs in groupby(variants, key=lambda x: x["type"]): + ${" | ".join("{}(ref value)".format(v["name"]) for v in vs)} => { value.size_of(ops) } % endfor @@ -365,8 +394,8 @@ impl PropertyDeclaration { let mut dest = CssWriter::new(dest); match *self { - % for ty, variants in groups.iteritems(): - ${" | ".join("{}(ref value)".format(v) for v in variants)} => { + % for ty, vs in groupby(variants, key=lambda x: x["type"]): + ${" | ".join("{}(ref value)".format(v["name"]) for v in vs)} => { value.to_css(&mut dest) } % endfor From 335cb4c9f4a48acdd82537f7d49019f0e5b34aa1 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Wed, 7 Feb 2018 22:48:44 +0100 Subject: [PATCH 2/3] Whitelist Copy types when generating PropertyDeclaration --- .../style/properties/properties.mako.rs | 68 +++++++++++++++---- components/style/values/generics/transform.rs | 8 +-- .../style/values/specified/background.rs | 2 +- components/style/values/specified/border.rs | 2 +- components/style/values/specified/font.rs | 6 +- 5 files changed, 63 insertions(+), 23 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 21d585fcb48..edd82ce6236 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -206,11 +206,51 @@ pub mod animated_properties { <% from itertools import groupby + copyTypes = set([ + "AlignContent", + "AlignItems", + "AlignSelf", + "BackgroundRepeat", + "BorderImageRepeat", + "BorderStyle", + "Contain", + "FontStyleAdjust", + "FontSynthesis", + "FontWeight", + "GridAutoFlow", + "ImageOrientation", + "InitialLetter", + "Integer", + "IntegerOrAuto", + "JustifyContent", + "JustifyItems", + "JustifySelf", + "MozForceBrokenImageIcon", + "MozScriptLevel", + "MozScriptMinSize", + "MozScriptSizeMultiplier", + "NonNegativeNumber", + "Opacity", + "OutlineStyle", + "OverscrollBehavior", + "Percentage", + "PositiveIntegerOrAuto", + "SVGPaintOrder", + "ScrollSnapType", + "TextDecorationLine", + "TouchAction", + "TransformStyle", + "XSpan", + "XTextZoom", + ]) + variants = [] for property in data.longhands: if property.predefined_type and not property.is_vector: + copy = property.predefined_type in copyTypes ty = "::values::specified::{}".format(property.predefined_type) else: + copy = bool(property.keyword) and not property.is_vector ty = "longhands::{}::SpecifiedValue".format(property.ident) if property.boxed: ty = "Box<{}>".format(ty) @@ -218,7 +258,7 @@ pub mod animated_properties { "name": property.camel_case, "type": ty, "doc": "`" + property.name + "`", - "keyword": bool(property.keyword) and not property.is_vector, + "copy": copy, }) groups = {} @@ -229,9 +269,9 @@ pub mod animated_properties { groups[ty] = group for v in group: if len(group) == 1: - sortkeys[v["name"]] = (not v["keyword"], 1, v["name"], "") + sortkeys[v["name"]] = (not v["copy"], 1, v["name"], "") else: - sortkeys[v["name"]] = (not v["keyword"], len(group), ty, v["name"]) + sortkeys[v["name"]] = (not v["copy"], len(group), ty, v["name"]) variants.sort(key=lambda x: sortkeys[x["name"]]) # It is extremely important to sort the `data.longhands` array here so @@ -251,19 +291,19 @@ pub mod animated_properties { "name": "CSSWideKeyword", "type": "WideKeywordDeclaration", "doc": "A CSS-wide keyword.", - "keyword": False, + "copy": False, }, { "name": "WithVariables", "type": "VariableDeclaration", "doc": "An unparsed declaration.", - "keyword": False, + "copy": False, }, { "name": "Custom", "type": "CustomDeclaration", "doc": "A custom property declaration.", - "keyword": False, + "copy": False, }, ] for v in extra: @@ -292,15 +332,15 @@ impl Clone for PropertyDeclaration { use self::PropertyDeclaration::*; <% - [keywords, others] = [list(g) for _, g in groupby(variants, key=lambda x: not x["keyword"])] + [copy, others] = [list(g) for _, g in groupby(variants, key=lambda x: not x["copy"])] %> match *self { - ${" | ".join("{}(..)".format(v["name"]) for v in keywords)} => { + ${" |\n".join("{}(..)".format(v["name"]) for v in copy)} => { #[derive(Clone, Copy)] #[repr(u16)] - enum Keywords { - % for v in keywords: + enum CopyVariants { + % for v in copy: _${v["name"]}(${v["type"]}), % endfor } @@ -308,8 +348,8 @@ impl Clone for PropertyDeclaration { unsafe { let mut out = mem::uninitialized(); ptr::write( - &mut out as *mut _ as *mut Keywords, - *(self as *const _ as *const Keywords), + &mut out as *mut _ as *mut CopyVariants, + *(self as *const _ as *const CopyVariants), ); out } @@ -323,7 +363,7 @@ impl Clone for PropertyDeclaration { ${vs[0]["name"]}(value.clone()) } % else: - ${" | ".join("{}(ref value)".format(v["name"]) for v in vs)} => { + ${" |\n".join("{}(ref value)".format(v["name"]) for v in vs)} => { unsafe { let mut out = ManuallyDrop::new(mem::uninitialized()); ptr::write( @@ -357,7 +397,7 @@ impl PartialEq for PropertyDeclaration { } match *self { % for ty, vs in groupby(variants, key=lambda x: x["type"]): - ${" | ".join("{}(ref this)".format(v["name"]) for v in vs)} => { + ${" |\n".join("{}(ref this)".format(v["name"]) for v in vs)} => { let other_repr = &*(other as *const _ as *const PropertyDeclarationVariantRepr<${ty}>); *this == other_repr.value diff --git a/components/style/values/generics/transform.rs b/components/style/values/generics/transform.rs index f2818842723..caa71d15102 100644 --- a/components/style/values/generics/transform.rs +++ b/components/style/values/generics/transform.rs @@ -675,8 +675,8 @@ pub fn get_normalized_vector_and_angle( } } -#[derive(ComputeSquaredDistance, ToAnimatedZero, ToComputedValue)] -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] +#[derive(Clone, ComputeSquaredDistance, Copy, Debug, MallocSizeOf, PartialEq)] +#[derive(ToAnimatedZero, ToComputedValue, ToCss)] /// A value of the `Rotate` property /// /// @@ -689,8 +689,8 @@ pub enum Rotate { Rotate3D(Number, Number, Number, Angle), } -#[derive(ComputeSquaredDistance, ToAnimatedZero, ToComputedValue)] -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] +#[derive(Clone, ComputeSquaredDistance, Copy, Debug, MallocSizeOf, PartialEq)] +#[derive(ToAnimatedZero, ToComputedValue, ToCss)] /// A value of the `Scale` property /// /// diff --git a/components/style/values/specified/background.rs b/components/style/values/specified/background.rs index e26414ca768..4387563a3b1 100644 --- a/components/style/values/specified/background.rs +++ b/components/style/values/specified/background.rs @@ -58,7 +58,7 @@ pub enum BackgroundRepeatKeyword { /// The specified value for the `background-repeat` property. /// /// https://drafts.csswg.org/css-backgrounds/#the-background-repeat -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] +#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToCss)] pub enum BackgroundRepeat { /// `repeat-x` RepeatX, diff --git a/components/style/values/specified/border.rs b/components/style/values/specified/border.rs index f382e1d4634..a9cc4fc2983 100644 --- a/components/style/values/specified/border.rs +++ b/components/style/values/specified/border.rs @@ -186,7 +186,7 @@ pub enum BorderImageRepeatKeyword { /// The specified value for the `border-image-repeat` property. /// /// https://drafts.csswg.org/css-backgrounds/#the-border-image-repeat -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] +#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToCss)] pub struct BorderImageRepeat(pub BorderImageRepeatKeyword, pub Option); impl BorderImageRepeat { diff --git a/components/style/values/specified/font.rs b/components/style/values/specified/font.rs index c823dca799c..50d703cfa06 100644 --- a/components/style/values/specified/font.rs +++ b/components/style/values/specified/font.rs @@ -1776,7 +1776,7 @@ impl Parse for FontFeatureSettings { } } -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue)] +#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToComputedValue)] /// Whether user agents are allowed to synthesize bold or oblique font faces /// when a font family lacks bold or italic faces pub struct FontSynthesis { @@ -2054,7 +2054,7 @@ impl Parse for VariationValue { } -#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue)] +#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToComputedValue)] /// text-zoom. Enable if true, disable if false pub struct XTextZoom(pub bool); @@ -2106,7 +2106,7 @@ impl ToCss for XLang { } #[cfg_attr(feature = "gecko", derive(MallocSizeOf))] -#[derive(Clone, Debug, PartialEq, ToCss)] +#[derive(Clone, Copy, Debug, PartialEq, ToCss)] /// Specifies the minimum font size allowed due to changes in scriptlevel. /// Ref: https://wiki.mozilla.org/MathML:mstyle pub struct MozScriptMinSize(pub NoCalcLength); From 5d8e70dc27557d0bf5ba7d0cbe6155158b3eca2b Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 8 Feb 2018 01:22:34 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Avoid=20pattern=20matching=20to=20clone=20C?= =?UTF-8?q?opy=20variants=20of=20PropertyDeclaration=20=F0=9F=90=89?= =?UTF-8?q?=F0=9F=90=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 1 + components/style/Cargo.toml | 1 + components/style/lib.rs | 1 + components/style/properties/data.py | 52 ++++++++++ .../style/properties/properties.mako.rs | 96 +++++++------------ 5 files changed, 88 insertions(+), 63 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5e22965bdfc..bb081b3e3f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2886,6 +2886,7 @@ dependencies = [ "byteorder 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "cfg-if 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "cssparser 0.23.2 (registry+https://github.com/rust-lang/crates.io-index)", + "debug_unreachable 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "encoding_rs 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", "fallible 0.0.1", diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 16d46eb6e26..cda14b1decf 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -32,6 +32,7 @@ bitflags = "1.0" byteorder = "1.0" cfg-if = "0.1.0" cssparser = "0.23.0" +debug_unreachable = "0.1.1" encoding_rs = {version = "0.7", optional = true} euclid = "0.16" fallible = { path = "../fallible" } diff --git a/components/style/lib.rs b/components/style/lib.rs index 5c5a837d5ba..429d813a754 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -33,6 +33,7 @@ extern crate bitflags; #[allow(unused_extern_crates)] extern crate byteorder; #[cfg(feature = "gecko")] #[macro_use] #[no_link] extern crate cfg_if; #[macro_use] extern crate cssparser; +#[macro_use] extern crate debug_unreachable; extern crate euclid; extern crate fallible; extern crate fnv; diff --git a/components/style/properties/data.py b/components/style/properties/data.py index ac7f9e427f7..4c9d2a066c7 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -226,6 +226,58 @@ class Longhand(object): def enabled_in_content(self): return self.enabled_in == "content" + def specified_type(self): + if self.predefined_type and not self.is_vector: + ty = "::values::specified::{}".format(self.predefined_type) + else: + ty = "longhands::{}::SpecifiedValue".format(self.ident) + if self.boxed: + ty = "Box<{}>".format(ty) + return ty + + def specified_is_copy(self): + if self.is_vector or self.boxed: + return False + if self.predefined_type: + return self.predefined_type in { + "AlignContent", + "AlignItems", + "AlignSelf", + "BackgroundRepeat", + "BorderImageRepeat", + "BorderStyle", + "Contain", + "FontStyleAdjust", + "FontSynthesis", + "FontWeight", + "GridAutoFlow", + "ImageOrientation", + "InitialLetter", + "Integer", + "IntegerOrAuto", + "JustifyContent", + "JustifyItems", + "JustifySelf", + "MozForceBrokenImageIcon", + "MozScriptLevel", + "MozScriptMinSize", + "MozScriptSizeMultiplier", + "NonNegativeNumber", + "Opacity", + "OutlineStyle", + "OverscrollBehavior", + "Percentage", + "PositiveIntegerOrAuto", + "SVGPaintOrder", + "ScrollSnapType", + "TextDecorationLine", + "TouchAction", + "TransformStyle", + "XSpan", + "XTextZoom", + } + return bool(self.keyword) + class Shorthand(object): def __init__(self, name, sub_properties, spec=None, servo_pref=None, gecko_pref=None, diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index edd82ce6236..aa78d329180 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -206,59 +206,22 @@ pub mod animated_properties { <% from itertools import groupby - copyTypes = set([ - "AlignContent", - "AlignItems", - "AlignSelf", - "BackgroundRepeat", - "BorderImageRepeat", - "BorderStyle", - "Contain", - "FontStyleAdjust", - "FontSynthesis", - "FontWeight", - "GridAutoFlow", - "ImageOrientation", - "InitialLetter", - "Integer", - "IntegerOrAuto", - "JustifyContent", - "JustifyItems", - "JustifySelf", - "MozForceBrokenImageIcon", - "MozScriptLevel", - "MozScriptMinSize", - "MozScriptSizeMultiplier", - "NonNegativeNumber", - "Opacity", - "OutlineStyle", - "OverscrollBehavior", - "Percentage", - "PositiveIntegerOrAuto", - "SVGPaintOrder", - "ScrollSnapType", - "TextDecorationLine", - "TouchAction", - "TransformStyle", - "XSpan", - "XTextZoom", - ]) + # After this code, `data.longhands` is sorted in the following order: + # - first all keyword variants and all variants known to be Copy, + # - second all the other variants, such as all variants with the same field + # have consecutive discriminants. + # The variable `variants` contain the same entries as `data.longhands` in + # the same order, but must exist separately to the data source, because + # we then need to add three additional variants `WideKeywordDeclaration`, + # `VariableDeclaration` and `CustomDeclaration`. variants = [] for property in data.longhands: - if property.predefined_type and not property.is_vector: - copy = property.predefined_type in copyTypes - ty = "::values::specified::{}".format(property.predefined_type) - else: - copy = bool(property.keyword) and not property.is_vector - ty = "longhands::{}::SpecifiedValue".format(property.ident) - if property.boxed: - ty = "Box<{}>".format(ty) variants.append({ "name": property.camel_case, - "type": ty, + "type": property.specified_type(), "doc": "`" + property.name + "`", - "copy": copy, + "copy": property.specified_is_copy(), }) groups = {} @@ -335,24 +298,31 @@ impl Clone for PropertyDeclaration { [copy, others] = [list(g) for _, g in groupby(variants, key=lambda x: not x["copy"])] %> + let self_tag = unsafe { + (*(self as *const _ as *const PropertyDeclarationVariantRepr<()>)).tag + }; + if self_tag <= LonghandId::${copy[-1]["name"]} as u16 { + #[derive(Clone, Copy)] + #[repr(u16)] + enum CopyVariants { + % for v in copy: + _${v["name"]}(${v["type"]}), + % endfor + } + + unsafe { + let mut out = mem::uninitialized(); + ptr::write( + &mut out as *mut _ as *mut CopyVariants, + *(self as *const _ as *const CopyVariants), + ); + return out; + } + } + match *self { ${" |\n".join("{}(..)".format(v["name"]) for v in copy)} => { - #[derive(Clone, Copy)] - #[repr(u16)] - enum CopyVariants { - % for v in copy: - _${v["name"]}(${v["type"]}), - % endfor - } - - unsafe { - let mut out = mem::uninitialized(); - ptr::write( - &mut out as *mut _ as *mut CopyVariants, - *(self as *const _ as *const CopyVariants), - ); - out - } + unsafe { debug_unreachable!() } } % for ty, vs in groupby(others, key=lambda x: x["type"]): <%