Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't generate useless InheritTypes interfaces #7735

Merged
merged 1 commit into from Oct 5, 2015

Conversation

@nox
Copy link
Member

nox commented Sep 25, 2015

Interfaces with no descendant need neither a Base trait nor upcast functions, and interfaces with no ancestors neither a Derived trait nor downcast functions.

Review on Reviewable

@nox nox force-pushed the nox:rm-useless-casts branch from d8cf701 to e3dab6f Sep 25, 2015
@nox nox force-pushed the nox:rm-useless-casts branch from e3dab6f to 3a5f330 Sep 27, 2015
@nox
Copy link
Member Author

nox commented Sep 27, 2015

I fixed the setting of hasProxyDescendant. This flag shouldn't be set on the proxy itself. This led to CSSStyleDeclaration's upcasts to be generated even though they are useless.

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 28, 2015

Reviewed 1 of 1 files at r1.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/bindings/codegen/CodegenRust.py, line 5812 [r2] (raw file):
I had issues wrapping my head around this, so I rewrote it:

        descriptors = config.getDescriptors(register=True, isCallback=False)
        allprotos = [CGGeneric("use dom::types::*;\n"),
                     CGGeneric("use dom::bindings::js::{JS, LayoutJS, Root};\n"),
                     CGGeneric("use dom::bindings::trace::JSTraceable;\n"),
                     CGGeneric("use dom::bindings::utils::Reflectable;\n"),
                     CGGeneric("use js::jsapi::JSTracer;\n\n"),
                     CGGeneric("use std::mem;\n\n")]

        allprotos.extend(CGGeneric(conversion)
                         for conversion in conversions(descriptor)
                         for descriptor in descriptors)

        curr = CGList(allprotos)
        curr = CGWrapper(curr, pre=AUTOGENERATED_WARNING_COMMENT)
        return curr


def conversions(descriptor):
    name = descriptor.name
    upcast = (descriptor.interface.getUserData("hasConcreteDescendant", False) or
              descriptor.interface.getUserData("hasProxyDescendant", False))

    downcast = len(descriptor.prototypeChain) > 1

    if upcast or downcast:
        yield "pub struct %sCast;\n" % name


    chain = descriptor.prototypeChain

    # Define a `FooBase` trait for subclasses to implement, as well as the
    # `FooCast::from_*` methods that use it.
    if upcast:
        yield string.Template("""\
/// Types which are derived from `${name}` and can be freely converted
/// to `${name}`
pub trait ${fromBound} : Sized {}

impl ${name}Cast {
    #[inline]
    /// Upcast an instance of a derived class of `${name}` to `${name}`
    pub fn from_ref<'a, T: ${fromBound}+Reflectable>(derived: &'a T) -> &'a ${name} {
        unsafe { mem::transmute(derived) }
    }
    #[inline]
    #[allow(unrooted_must_root)]
    pub fn from_layout_js<T: ${fromBound}+Reflectable>(derived: &LayoutJS<T>) -> LayoutJS<${name}> {
        unsafe { mem::transmute_copy(derived) }
    }
    #[inline]
    pub fn from_root<T: ${fromBound}+Reflectable>(derived: Root<T>) -> Root<${name}> {
        unsafe { mem::transmute(derived) }
    }
}
""").substitute({'name': name, 'fromBound': name + 'Base'})
    else:
        # The `FooBase` trait is not defined, so avoid implementing it by
        # removing `Foo` itself from the chain.
        chain = chain[:-1]

    # Implement `BarBase` for `Foo`, for all `Bar` that `Foo` inherits from.
    for proto in chain:
        yield 'impl %s for %s {}\n' % (proto + 'Base', descriptor.concreteType)


    # Define a `FooDerived` trait for superclasses to implement, as well as the
    # `FooCast::to_*` methods that use it.
    if downcast:
        yield string.Template("""\
/// Types which `${name}` derives from
pub trait ${toBound} : Sized { fn ${checkFn}(&self) -> bool; }

impl %{name}Cast {
    #[inline]
    /// Downcast an instance of a base class of `${name}` to an instance of
    /// `${name}`, if it internally is an instance of `${name}`
    pub fn to_ref<'a, T: ${toBound}+Reflectable>(base: &'a T) -> Option<&'a ${name}> {
        match base.${checkFn}() {
            true => Some(unsafe { mem::transmute(base) }),
            false => None
        }
    }
    #[inline]
    #[allow(unrooted_must_root)]
    pub fn to_layout_js<T: ${toBound}+Reflectable>(base: &LayoutJS<T>) -> Option<LayoutJS<${name}>> {
        unsafe {
            match (*base.unsafe_get()).${checkFn}() {
                true => Some(mem::transmute_copy(base)),
                false => None
            }
        }
    }
    #[inline]
    pub fn to_root<T: ${toBound}+Reflectable>(base: Root<T>) -> Option<Root<${name}>> {
        match base.r().${checkFn}() {
            true => Some(unsafe { mem::transmute(base) }),
            false => None
        }
    }
}
""").substitute({'checkFn': 'is_' + name.lower(), 'name': name, 'toBound': name + 'Derived'})


    # Implement the `FooDerived` trait for non-root superclasses by deferring to
    # the direct superclass. This leaves the implementation of the `FooDerived`
    # trait for the root superclass to manual code. `FooDerived` is not
    # implemented for `Foo` itself. 
    for protoName in descriptor.prototypeChain[1:-1]:
        protoDescriptor = config.getDescriptor(protoName)
        yield string.Template("""\
impl ${name}Derived for ${baseName} {
    #[inline]
    fn ${fname}(&self) -> bool {
        let base: &${parentName} = ${parentName}Cast::from_ref(self);
        base.${fname}()
    }
}
""").substitute({'fname': 'is_' + name.lower(),
                 'name': name,
                 'baseName': protoDescriptor.concreteType,
                 'parentName': protoDescriptor.prototypeChain[-2]})

How would you feel about me rewriting the code in this fashion, and letting you rebase on top of it?


components/script/dom/bindings/codegen/CodegenRust.py, line 5904 [r2] (raw file):
This doesn't use checkFn


Comments from the review on Reviewable.io

@nox
Copy link
Member Author

nox commented Sep 28, 2015

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/bindings/codegen/CodegenRust.py, line 5812 [r2] (raw file):
Sure.


Comments from the review on Reviewable.io

@nox
Copy link
Member Author

nox commented Sep 28, 2015

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/bindings/codegen/CodegenRust.py, line 5812 [r2] (raw file):

17:31 Ms2ger: Actually I have a problem with your rewrite, in my other patch I build a dictionary from interface names to their immediate descendants, to generate the TypeId enums,
17:31 Ms2ger: your generator rewrite forbids me to build this dictionary, AFAICT.

It forbids me to build this dictionary while generating the plain old conversions functions, I mean.


Comments from the review on Reviewable.io

@nox
Copy link
Member Author

nox commented Sep 28, 2015

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/bindings/codegen/CodegenRust.py, line 5812 [r2] (raw file):
master...nox:codegen-typeid


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:rm-useless-casts branch from 3a5f330 to 59f40b6 Oct 2, 2015
Interfaces with no descendant need neither a Base trait nor upcast functions,
and interfaces with no ancestors neither a Derived trait nor downcast functions.
@nox nox force-pushed the nox:rm-useless-casts branch from 59f40b6 to e24b8ab Oct 2, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 5, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

📌 Commit e24b8ab has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

Testing commit e24b8ab with merge 8db54e4...

bors-servo pushed a commit that referenced this pull request Oct 5, 2015
Don't generate useless InheritTypes interfaces

Interfaces with no descendant need neither a Base trait nor upcast functions, and interfaces with no ancestors neither a Derived trait nor downcast functions.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

💔 Test failed - mac-dev-ref-unit

@nox
Copy link
Member Author

nox commented Oct 5, 2015

@bors-servo retry p=1

#7785

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

Testing commit e24b8ab with merge 243647d...

bors-servo pushed a commit that referenced this pull request Oct 5, 2015
Don't generate useless InheritTypes interfaces

Interfaces with no descendant need neither a Base trait nor upcast functions, and interfaces with no ancestors neither a Derived trait nor downcast functions.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

💔 Test failed - linux-rel

@nox
Copy link
Member Author

nox commented Oct 5, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2015

@bors-servo bors-servo merged commit e24b8ab into servo:master Oct 5, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:rm-useless-casts branch Dec 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.