Introduce safer layer of sugar for nsStyleUnion by Manishearth · Pull Request #12521 · servo/servo

Manishearth · 2016-07-20T13:07:39Z

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

This change is

highfive · 2016-07-20T13:07:44Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/gecko_values.rs

highfive · 2016-07-20T13:07:45Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

Manishearth · 2016-07-20T13:10:11Z

Oh, another issue is that we need to make all unit/union fields private and add unsafe accessors. This will need to be done through bindgen changes, which I'll need to look into.

For now, we're directly setting the mUnit/mValue fields only for z-index, and there's a justifying comment.

Manishearth · 2016-07-20T13:37:42Z

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs

+                    *(*self.union).mInt.as_mut() = i as i32;
+                }
+                Calc(calc) => {
+                    *self.unit = eStyleUnit_Calc;


This line shouldn't be here, since we assert otherwise in set_calc_value.

Manishearth · 2016-07-20T13:50:56Z

I went ahead and made the traits changes. They're in a separate commit so that we can still pick the old way of doing it if we want to.

It might be possible to clean up some of the impls in that file with macros. Not sure if it's necessary though.

Manishearth · 2016-07-20T16:25:30Z

rust-lang/rust-bindgen#20 will make it possible to make this even more safe 😀

bholley · 2016-07-20T18:29:28Z

So much nicer!

r=me with those comments addressed, but please smoketest this a bit. :-)

@bors-servo delegate+

Reviewed 1 of 1 files at r1, 1 of 4 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

components/style/properties/gecko.mako.rs, line 371 [r3] (raw file):

    pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
        v.0.width.to_gecko_style_coord(&mut self.gecko.${gecko_ffi_name}.data_at_mut(${x_index}));
        v.0.width.to_gecko_style_coord(&mut self.gecko.${gecko_ffi_name}.data_at_mut(${y_index}));

Don't you mean height here?

components/style/properties/gecko.mako.rs, line 377 [r3] (raw file):

        self.gecko.${gecko_ffi_name}.data_at_mut(${x_index})
                  .copy_from(&other.gecko.${gecko_ffi_name}.data_at(${x_index}));
        self.gecko.${gecko_ffi_name}.data_at_mut(${x_index})

And don't you want y_index here?

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 188 [r2] (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

This line shouldn't be here, since we assert otherwise in set_calc_value.

I don't see where Gecko_SetStyleCoordCalcValue is implemented (maybe hasn't landed yet?), but I'd think we should only pass the union and let servo take care of setting the unit. But why do we need this function at all? Given that we're already reset()ing above, can't we just set the pointer and call the FFI function to addref?

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 53 [r3] (raw file):

impl<'a> CoordData for SidesData<'a> {
    fn unit(&self) -> nsStyleUnit {
        self.sides.mUnits[self.index]

Given that this isn't marked unsafe, I'm presuming that something in the rust type system guarantees that we can't have index > 3. Out of curiosity, how does that work?

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 122 [r3] (raw file):

#[derive(Copy, Clone)]
/// Enum representing the tagged union that is CoordData

The general commenting style we've been using so far uses periods in places like this (like Gecko does). Unless there's a rust convention to omit them, I'd rather be consistent. This appears in various places in this PR.

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 125 [r3] (raw file):

CoordDataValues

I'd think we should call this CoordDataValue?

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 177 [r3] (raw file):

    #[inline(always)]
    fn set_enum(&mut self, value: CoordDataValues) {

Given that 'enum' is one of the values of the enumerated type itself, I'd prefer to just call this something more generic like "set".

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 250 [r3] (raw file):

    #[inline]
    unsafe fn as_calc_mut(&mut self) -> &mut nsStyleCoord_Calc {
        transmute(*self.union().mPointer.as_mut() as *mut nsStyleCoord_Calc)

Can we debug_assert the unit here?

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 268 [r3] (raw file):

    #[inline(always)]
    fn as_enum(&self) -> CoordDataValues {

Same thing with the word 'enum' - maybe .get() or .as_value()? Something consistent with what we do for set.

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 296 [r3] (raw file):

    /// While this should usually be called with the unit checked,
    /// it is not an intrinsically unsafe operation to call this function
    /// with the wrong unit

Aside from the fact that we're interpreting potentially-garbage memory as a float? That seems bad enough that I would nix this comment and add debug_asserts to all these methods. We might as well just mark them unsafe, because the only callers are already in unsafe blocks.

ports/geckolib/gecko_bindings/sugar/ns_style_coord.rs, line 319 [r3] (raw file):

as_calc

The distinction between as_calc and get_calc is weird. Maybe we should call these get_calc and get_calc_value respectively?

Comments from Reviewable

bors-servo · 2016-07-20T18:29:30Z

✌️ @Manishearth can now approve this pull request

Manishearth · 2016-07-21T07:21:07Z

Review status: 2 of 6 files reviewed at latest revision, 11 unresolved discussions.

components/style/properties/gecko.mako.rs, line 371 [r3] (raw file):