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

Derive DomObject with a proc macro #15543

Merged
merged 1 commit into from Feb 14, 2017
Merged

Derive DomObject with a proc macro #15543

merged 1 commit into from Feb 14, 2017

Conversation

@nox
Copy link
Member

nox commented Feb 14, 2017

This change is Reviewable

@highfive
Copy link

highfive commented Feb 14, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/bindings/reflector.rs
  • @KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/bindings/reflector.rs
@highfive
Copy link

highfive commented Feb 14, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@SimonSapin
Copy link
Member

SimonSapin commented Feb 14, 2017

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/domobject_derive/lib.rs, line 41 at r1 (raw file):

            #[inline]
            fn reflector(&self) -> &::dom::bindings::reflector::Reflector {
                self.#first_field_name.reflector()

Change this to a fully qualified call? ::dom::bindings::reflector::DomObject::reflector(&self.#first_field_name). Same for init_reflector below. r=me with that.


Comments from Reviewable

@nox
Copy link
Member Author

nox commented Feb 14, 2017

A trait is always imported in itself, so it's not a problem to not fully-qualify that.

@bors-servo r=SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

📌 Commit c84cea9 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

Testing commit c84cea9 with merge 2af79b8...

bors-servo added a commit that referenced this pull request Feb 14, 2017
Derive DomObject with a proc macro

<!-- 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/15543)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: SimonSapin
Pushing 2af79b8 to master...

@bors-servo bors-servo merged commit c84cea9 into servo:master Feb 14, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:plugin branch Feb 14, 2017
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 14, 2017

Change this to a fully qualified call? ::dom::bindings::reflector::DomObject::reflector(&self.#first_field_name). Same for init_reflector below. r=me with that.

I think we should still do this, to ensure that it is indeed a DomObject method we're calling.

@nox
Copy link
Member Author

nox commented Feb 14, 2017

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

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