Auto merge of #12521 - Manishearth:safer-coord, r=bholley

Introduce safer layer of sugar for nsStyleUnion

This routes (almost) all access to nsStyleUnion through a largely safe interface, CoordData.

It also introduces a corresponding Rust enum, CoordDataValue, which can be used when interacting with CoordData

LLVM should optimize the costs away in release mode. eddyb tested this a bit, and LLVM has no trouble threading matches together with inlining -- so all of the matches using enums here will have the same generated code as the old matches on the units.

Some unresolved questions:

Should I provide convenience methods like `set_coord`, `set_auto`, etc on CoordData? `.set_enum(CoordDataValue::Something)` should optimize down to the same thing, but the convenience methods look cleaner and won't load the optimizer as much.

Also, we're converting immutable references to mutable ones, which can be used to cause unsafety using some acrobatics. Perhaps a trait-based approach is better?
The issue is that in some places we only have a `&CoordData` (eg copy_from), but CoordData internally is `*mut`. It would be nice if CoordData could parametrize over its mutability, but Rust doesn't let us do that.

The alternate approach is to make CoordData a trait (also CoordDataMut). The trait requires you to implement `get_union()` and `get_unit()`, and gives you the rest of the functions for free. `nsStyleCoord` would directly implement both traits. `nsStyleSides` would have `data_at(idx)` and `data_at_mut(idx)` methods which return a struct `SidesData` containing a reference to the Sides and the index, which itself implements the `CoordData` and `CoordDataMut` trait (we need two traits here because there will have to be two `SidesData` structs).

I decided not to implement the traits approach first since it's pretty trivial to change this code to use traits, and the current design is more straightforward.

Thoughts?

r? @bholley

cc @emilio @heycam

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12521)
<!-- Reviewable:end -->

ports/geckolib/gecko_bindings/sugar/mod.rs

@@ -3,5 +3,5 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 mod ns_style_auto_array;
-mod ns_style_coord;
+pub mod ns_style_coord;
 mod ns_t_array;

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs

@@ -4,51 +4,330 @@
 
 use bindings::{Gecko_ResetStyleCoord, Gecko_SetStyleCoordCalcValue, Gecko_AddRefCalcArbitraryThread};
 use std::mem::transmute;
-use structs::{nsStyleCoord_CalcValue, nsStyleCoord_Calc, nsStyleUnit, nsStyleUnion, nsStyleCoord};
+use structs::{nsStyleCoord_Calc, nsStyleUnit, nsStyleUnion, nsStyleCoord, nsStyleSides, nsStyleCorners};
+use structs::{nsStyleCoord_CalcValue, nscoord};
 
-// Functions here are unsafe because it is possible to use the wrong nsStyleUnit
-// FIXME we should be pairing up nsStyleUnion and nsStyleUnit somehow
-// nsStyleCoord is one way to do it, but there are other structs using pairs
-// of union and unit too
+impl CoordData for nsStyleCoord {
+    #[inline]
+    fn unit(&self) -> nsStyleUnit {
+        self.mUnit
+    }
+    #[inline]
+    fn union(&self) -> nsStyleUnion {
+        self.mValue
+    }
+}
 
-impl nsStyleUnion {
-    /// Clean up any resources used by an nsStyleUnit
+impl CoordDataMut for nsStyleCoord {
+    unsafe fn values_mut(&mut self) -> (&mut nsStyleUnit, &mut nsStyleUnion) {
+        (&mut self.mUnit, &mut self.mValue)
+    }
+}
+
+impl nsStyleSides {
+    #[inline]
+    pub fn data_at(&self, index: usize) -> SidesData {
+        SidesData {
+            sides: self,
+            index: index,
+        }
+    }
+    #[inline]
+    pub fn data_at_mut(&mut self, index: usize) -> SidesDataMut {
+        SidesDataMut {
+            sides: self,
+            index: index,
+        }
+    }
+}
+
+pub struct SidesData<'a> {
+    sides: &'a nsStyleSides,
+    index: usize,
+}
+pub struct SidesDataMut<'a> {
+    sides: &'a mut nsStyleSides,
+    index: usize,
+}
+
+impl<'a> CoordData for SidesData<'a> {
+    #[inline]
+    fn unit(&self) -> nsStyleUnit {
+        self.sides.mUnits[self.index]
+    }
+    #[inline]
+    fn union(&self) -> nsStyleUnion {
+        self.sides.mValues[self.index]
+    }
+}
+impl<'a> CoordData for SidesDataMut<'a> {
+    #[inline]
+    fn unit(&self) -> nsStyleUnit {
+        self.sides.mUnits[self.index]
+    }
+    #[inline]
+    fn union(&self) -> nsStyleUnion {
+        self.sides.mValues[self.index]
+    }
+}
+impl<'a> CoordDataMut for SidesDataMut<'a> {
+    unsafe fn values_mut(&mut self) -> (&mut nsStyleUnit, &mut nsStyleUnion) {
+        (&mut self.sides.mUnits[self.index], &mut self.sides.mValues[self.index])
+    }
+}
+
+impl nsStyleCorners {
+    #[inline]
+    pub fn data_at(&self, index: usize) -> CornersData {
+        CornersData {
+            corners: self,
+            index: index,
+        }
+    }
+    #[inline]
+    pub fn data_at_mut(&mut self, index: usize) -> CornersDataMut {
+        CornersDataMut {
+            corners: self,
+            index: index,
+        }
+    }
+}
+
+pub struct CornersData<'a> {
+    corners: &'a nsStyleCorners,
+    index: usize,
+}
+pub struct CornersDataMut<'a> {
+    corners: &'a mut nsStyleCorners,
+    index: usize,
+}
+
+impl<'a> CoordData for CornersData<'a> {
+    fn unit(&self) -> nsStyleUnit {
+        self.corners.mUnits[self.index]
+    }
+    fn union(&self) -> nsStyleUnion {
+        self.corners.mValues[self.index]
+    }
+}
+impl<'a> CoordData for CornersDataMut<'a> {
+    fn unit(&self) -> nsStyleUnit {
+        self.corners.mUnits[self.index]
+    }
+    fn union(&self) -> nsStyleUnion {
+        self.corners.mValues[self.index]
+    }
+}
+impl<'a> CoordDataMut for CornersDataMut<'a> {
+    unsafe fn values_mut(&mut self) -> (&mut nsStyleUnit, &mut nsStyleUnion) {
+        (&mut self.corners.mUnits[self.index], &mut self.corners.mValues[self.index])
+    }
+}
+
+#[derive(Copy, Clone)]
+/// Enum representing the tagged union that is CoordData.
+/// In release mode this should never actually exist in the code,
+/// and will be optimized out by threading matches and inlining.
+pub enum CoordDataValue {
+    Null,
+    Normal,
+    Auto,
+    None,
+    Percent(f32),
+    Factor(f32),
+    Degree(f32),
+    Grad(f32),
+    Radian(f32),
+    Turn(f32),
+    FlexFraction(f32),
+    Coord(nscoord),
+    Integer(i32),
+    Enumerated(u32),
+    Calc(nsStyleCoord_CalcValue),
+}
+
+
+pub trait CoordDataMut : CoordData {
+    // This can't be two methods since we can't mutably borrow twice
+    /// This is unsafe since it's possible to modify
+    /// the unit without changing the union
+    unsafe fn values_mut(&mut self) -> (&mut nsStyleUnit, &mut nsStyleUnion);
+
+    /// Clean up any resources used by the union
     /// Currently, this only happens if the nsStyleUnit
     /// is a Calc
-    pub unsafe fn reset(&mut self, unit: &mut nsStyleUnit) {
-        if *unit == nsStyleUnit::eStyleUnit_Calc {
-            Gecko_ResetStyleCoord(unit, self);
+    #[inline]
+    fn reset(&mut self) {
+        unsafe {
+            if self.unit() == nsStyleUnit::eStyleUnit_Calc {
+                let (unit, union) = self.values_mut();
+                Gecko_ResetStyleCoord(unit, union);
+            }
         }
     }
 
-    /// Set internal value to a calc() value
-    /// reset() the union before calling this
-    pub unsafe fn set_calc_value(&mut self, unit: &mut nsStyleUnit, v: nsStyleCoord_CalcValue) {
-        // Calc should have been cleaned up
-        debug_assert!(*unit != nsStyleUnit::eStyleUnit_Calc);
-        Gecko_SetStyleCoordCalcValue(unit, self, v);
+    #[inline]
+    fn copy_from<T: CoordData>(&mut self, other: &T) {
+        unsafe {
+            self.reset();
+            {
+                let (unit, union) = self.values_mut();
+                *unit = other.unit();
+                *union = other.union();
+            }
+            self.addref_if_calc();
+        }
     }
 
-    pub unsafe fn get_calc(&self) -> nsStyleCoord_CalcValue {
+    #[inline(always)]
+    fn set_value(&mut self, value: CoordDataValue) {
+        use self::CoordDataValue::*;
+        use structs::nsStyleUnit::*;
+        self.reset();
+        unsafe {
+            let (unit, union) = self.values_mut();
+            match value {
+                Null => {
+                    *unit = eStyleUnit_Null;
+                    *union.mInt.as_mut() = 0;
+                }
+                Normal => {
+                    *unit = eStyleUnit_Normal;
+                    *union.mInt.as_mut() = 0;
+                }
+                Auto => {
+                    *unit = eStyleUnit_Auto;
+                    *union.mInt.as_mut() = 0;
+                }
+                None => {
+                    *unit = eStyleUnit_None;
+                    *union.mInt.as_mut() = 0;
+                }
+                Percent(f) => {
+                    *unit = eStyleUnit_Percent;
+                    *union.mFloat.as_mut() = f;
+                }
+                Factor(f) => {
+                    *unit = eStyleUnit_Factor;
+                    *union.mFloat.as_mut() = f;
+                }
+                Degree(f) => {
+                    *unit = eStyleUnit_Degree;
+                    *union.mFloat.as_mut() = f;
+                }
+                Grad(f) => {
+                    *unit = eStyleUnit_Grad;
+                    *union.mFloat.as_mut() = f;
+                }
+                Radian(f) => {
+                    *unit = eStyleUnit_Radian;
+                    *union.mFloat.as_mut() = f;
+                }
+                Turn(f) => {
+                    *unit = eStyleUnit_Turn;
+                    *union.mFloat.as_mut() = f;
+                }
+                FlexFraction(f) => {
+                    *unit = eStyleUnit_FlexFraction;
+                    *union.mFloat.as_mut() = f;
+                }
+                Coord(coord) => {
+                    *unit = eStyleUnit_Coord;
+                    *union.mInt.as_mut() = coord;
+                }
+                Integer(i) => {
+                    *unit = eStyleUnit_Integer;
+                    *union.mInt.as_mut() = i;
+                }
+                Enumerated(i) => {
+                    *unit = eStyleUnit_Enumerated;
+                    *union.mInt.as_mut() = i as i32;
+                }
+                Calc(calc) => {
+                    // Gecko_SetStyleCoordCalcValue changes the unit internally
+                    Gecko_SetStyleCoordCalcValue(unit, union, calc);
+                }
+            }
+        }
+    }
+
+    #[inline]
+    unsafe fn as_calc_mut(&mut self) -> &mut nsStyleCoord_Calc {
+        debug_assert!(self.unit() == nsStyleUnit::eStyleUnit_Calc);
+        transmute(*self.union().mPointer.as_mut() as *mut nsStyleCoord_Calc)
+    }
+
+    #[inline]
+    fn addref_if_calc(&mut self) {
+        unsafe {
+            if self.unit() == nsStyleUnit::eStyleUnit_Calc {
+                Gecko_AddRefCalcArbitraryThread(self.as_calc_mut());
+            }
+        }
+    }
+}
+pub trait CoordData {
+    fn unit(&self) -> nsStyleUnit;
+    fn union(&self) -> nsStyleUnion;
+
+
+    #[inline(always)]
+    fn as_value(&self) -> CoordDataValue {
+        use self::CoordDataValue::*;
+        use structs::nsStyleUnit::*;
+        unsafe {
+            match self.unit() {
+                eStyleUnit_Null => Null,
+                eStyleUnit_Normal => Normal,
+                eStyleUnit_Auto => Auto,
+                eStyleUnit_None => None,
+                eStyleUnit_Percent => Percent(self.get_float()),
+                eStyleUnit_Factor => Factor(self.get_float()),
+                eStyleUnit_Degree => Degree(self.get_float()),
+                eStyleUnit_Grad => Grad(self.get_float()),
+                eStyleUnit_Radian => Radian(self.get_float()),
+                eStyleUnit_Turn => Turn(self.get_float()),
+                eStyleUnit_FlexFraction => FlexFraction(self.get_float()),
+                eStyleUnit_Coord => Coord(self.get_integer()),
+                eStyleUnit_Integer => Integer(self.get_integer()),
+                eStyleUnit_Enumerated => Enumerated(self.get_integer() as u32),
+                eStyleUnit_Calc => Calc(self.get_calc_value()),
+            }
+        }
+    }
+
+    #[inline]
+    /// Pretend inner value is a float; obtain it.
+    unsafe fn get_float(&self) -> f32 {
+        use structs::nsStyleUnit::*;
+        debug_assert!(self.unit() == eStyleUnit_Percent || self.unit() == eStyleUnit_Factor
+                      || self.unit() == eStyleUnit_Degree || self.unit() == eStyleUnit_Grad
+                      || self.unit() == eStyleUnit_Radian || self.unit() == eStyleUnit_Turn
+                      || self.unit() == eStyleUnit_FlexFraction);
+        *self.union().mFloat.as_ref()
+    }
+
+    #[inline]
+    /// Pretend inner value is an int; obtain it.
+    unsafe fn get_integer(&self) -> i32 {
+        use structs::nsStyleUnit::*;
+        debug_assert!(self.unit() == eStyleUnit_Coord || self.unit() == eStyleUnit_Integer
+                      || self.unit() == eStyleUnit_Enumerated);
+        *self.union().mInt.as_ref()
+    }
+
+    #[inline]
+    /// Pretend inner value is a calc; obtain it.
+    /// Ensure that the unit is Calc before calling this.
+    unsafe fn get_calc_value(&self) -> nsStyleCoord_CalcValue {
+        debug_assert!(self.unit() == nsStyleUnit::eStyleUnit_Calc);
         (*self.as_calc())._base
     }
 
-    pub unsafe fn addref_if_calc(&mut self, unit: &nsStyleUnit) {
-        if *unit == nsStyleUnit::eStyleUnit_Calc {
-            Gecko_AddRefCalcArbitraryThread(self.as_calc_mut());
-        }
-    }
 
-    unsafe fn as_calc_mut(&mut self) -> &mut nsStyleCoord_Calc {
-        transmute(*self.mPointer.as_mut() as *mut nsStyleCoord_Calc)
-    }
+    #[inline]
     unsafe fn as_calc(&self) -> &nsStyleCoord_Calc {
-        transmute(*self.mPointer.as_ref() as *const nsStyleCoord_Calc)
-    }
-}
-
-impl nsStyleCoord {
-    pub unsafe fn addref_if_calc(&mut self) {
-        self.mValue.addref_if_calc(&self.mUnit);
+        debug_assert!(self.unit() == nsStyleUnit::eStyleUnit_Calc);
+        transmute(*self.union().mPointer.as_ref() as *const nsStyleCoord_Calc)
     }
 }

ports/geckolib/gecko_bindings/tools/regen.py

@@ -94,7 +94,8 @@ COMPILATION_TARGETS = {
             "imgRequestProxy", "imgRequestProxyStatic", "CounterStyleManager",
             "ImageValue", "URLValue", "URLValueData", "nsIPrincipal",
             "nsDataHashtable", "imgIRequest"
-        ]
+        ],
+        "unsafe_field_types": ["nsStyleUnion", "nsStyleUnit"],
     },
     # Generation of the ffi bindings.
     "bindings": {
@@ -130,7 +131,7 @@ COMPILATION_TARGETS = {
         ],
         "void_types": [
             "nsINode", "nsIDocument", "nsIPrincipal", "nsIURI",
-        ]
+        ],
     }
 }
 
@@ -253,6 +254,11 @@ def build(objdir, target_name, kind_name=None,
             flags.append("-match")
             flags.append(header.format(objdir))
 
+    if "unsafe_field_types" in current_target:
+        for ty in current_target["unsafe_field_types"]:
+            flags.append("-unsafe-field-type")
+            flags.append(ty.format(objdir))
+
     if "blacklist" in current_target:
         for ty in current_target["blacklist"]:
             flags.append("-blacklist-type")