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

Update anonymous box list #12608

Merged
merged 3 commits into from Jul 27, 2016
Merged

Update anonymous box list #12608

merged 3 commits into from Jul 27, 2016

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Jul 26, 2016

These have drifted since we introduced them. Adding a few other misc commits while I'm at it.


This change is Reviewable

This handles the changes in bug 1277131 and bug 1097499, and should
allow us to remove the hacky fallback for anonymous boxes in stylo.
@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 26, 2016
@bholley
Copy link
Contributor Author

bholley commented Jul 26, 2016

r? @emilio

@highfive highfive assigned emilio and unassigned metajack Jul 26, 2016
@emilio
Copy link
Member

emilio commented Jul 26, 2016

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 2784f44 has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 26, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 2784f44 with merge c1409af...

bors-servo pushed a commit that referenced this pull request Jul 27, 2016
Update anonymous box list

These have drifted since we introduced them. Adding a few other misc commits while I'm at it.

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@bors-servo bors-servo merged commit 2784f44 into servo:master Jul 27, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 27, 2016
@Manishearth
Copy link
Member

The llvm_stable thing breaks the binding generation for me. If I pass it llvm38, I get this error:

  "_clang_Type_getNamedType", referenced from:
      bindgen::clang::Type::named::h017a16ee858af8af in libbindgen.rlib(bindgen.0.o)
  "_clang_Cursor_isFunctionInlined", referenced from:
      bindgen::clang::Cursor::is_inlined_function::hbf3ecb6d1f288ea8 in libbindgen.rlib(bindgen.0.o)

If I pass it my local llvm trunk build, it works but makes changes like:

 -- a/ports/geckolib/gecko_bindings/bindings.rs
 ++ b/ports/geckolib/gecko_bindings/bindings.rs
@@ -147,7 +147,7 @@ use structs::nsIAtom;

 pub type RawGeckoNode = nsINode;
 pub enum Element { }
- pub type RawGeckoElement = Element;
+ pub type RawGeckoElement = ::std::os::raw::c_void;
 pub type RawGeckoDocument = nsIDocument;
 pub enum ServoNodeData { }
 pub enum ServoComputedValues { }

(all mozilla:: things get replaced with void)

@bholley
Copy link
Contributor Author

bholley commented Jul 28, 2016

Hm, _clang_Cursor_isFunctionInlined and friends are LLVM39 symbols. The whole point of llvm_stable is that it avoids requiring llvm39 symbols, because we need to be on LLVM38 to have any chance at all of making LLVM a build-time requirement for Firefox.

Can you debug why llvm_stable seems to be having the opposite effect as intended?

@Manishearth
Copy link
Member

I'll try, sure 😄

a clean build of bindgen didn't seem to change anything but I should try this again.

@Manishearth
Copy link
Member

Works now. I cleaned it again, reset env vars, and it's fine.

@bholley bholley deleted the sync_anon_box branch August 26, 2016 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants