Skip to content

/ servo

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

Test that Servo_* functions have the right signatures #13620

Merged
merged 1 commit into from Oct 9, 2016
Merged

Test that Servo_* functions have the right signatures #13620

merged 1 commit into from Oct 9, 2016

Conversation

@Manishearth
Copy link
Member

Manishearth commented Oct 6, 2016 •
edited by larsbergstrom

Fixes #12992

Needs #13617

Not very happy with this solution (and perhaps it should be done in pure Rust, though that can be split out as another easy bug).

But it works. The bindings changes are from running a regen on bug 1308234

r? @emilio

This change is Reviewable
@highfive
Copy link

highfive commented Oct 6, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko_bindings/bindings.rs, components/style/properties/gecko.mako.rs, components/style/gecko_bindings/check_bindings.rs, components/style/binding_tools/regen.sh
@highfive
Copy link

highfive commented Oct 6, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@Manishearth Manishearth force-pushed the Manishearth:type-assert branch from c45b5e2 to 0da52b8 Oct 6, 2016
@emilio
emilio reviewed Oct 6, 2016
View changes
components/style/binding_tools/regen.sh Outdated
@@ -34,4 +34,4 @@ else
LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
fi

"./${TOOLS_DIR}/regen.py" --target all "${@}"
"./${TOOLS_DIR}/regen.py" --target bindings "${@}"

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 6, 2016

Member

This will stop regenerating structs, do you really want that?

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Oct 6, 2016

Author Member

Oh, that was a mistake. I do this locally since regen.py doesn't set up the env vars.
tests/unit/stylo/check_bindings.py

with open(GLUE_FILE, "r") as glue, open(GLUE_OUTPUT_FILE, "w+") as glue_output:
for line in glue:
glue_output.write(line.replace("pub extern \"C\" fn", "pub unsafe extern \"C\" fn"))

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 6, 2016

Member

So to make clear I've understood this right, the alternative to this hacky script is marking the functions as actually unsafe, right? It seems to me that we might as well do that and, if we care about the loss of compiler diagnostics, move the implementation to a safe function?

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Oct 6, 2016

Author Member

We could, I guess. Gets a bit cumbersome and in that case the macro-based solution is better. I prefer the hacky script to making it harder to write glue code. I will in the meantime try to get some coercions into rustc.
@Manishearth Manishearth force-pushed the Manishearth:type-assert branch 2 times, most recently from 3335857 to 2930bd0 Oct 6, 2016
@Manishearth
Test that Servo_* functions have the right signatures
Loading status checks…
af57a98 
Fixes #12992
@Manishearth
Copy link
Member Author

Manishearth commented Oct 9, 2016

Addressed
@Manishearth Manishearth force-pushed the Manishearth:type-assert branch from 2930bd0 to af57a98 Oct 9, 2016
@Manishearth
Copy link
Member Author

Manishearth commented Oct 9, 2016

@bors-servo r=emilio
@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2016

📌 Commit af57a98 has been approved by emilio
@emilio
Copy link
Member

emilio commented Oct 9, 2016 •
edited

So, I've just discussed with @Manishearth over IRC.

I'm really not too fond in this approach and would prefer converting all the extern functions in glue.rs to unsafe, then use non-unsafe implementations. Manish didn't agree with this, and thinks that we shouldn't hacks like the test code.

In any case, I guess this is fine to land as-is.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2016

⌛ Testing commit af57a98 with merge a0e404c...
bors-servo added a commit that referenced this pull request Oct 9, 2016
@bors-servo
Auto merge of #13620 - Manishearth:type-assert, r=emilio
Loading status checks…
a0e404c 
Test that Servo_* functions have the right signatures

Fixes #12992

Needs #13617

Not very happy with this solution (and perhaps it should be done in pure Rust, though that can be split out as another easy bug).

But it works. The bindings changes are from running a regen on [bug 1308234](https://bugzilla.mozilla.org/show_bug.cgi?id=1308234)

r? @emilio

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

bors-servo commented Oct 9, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit af57a98 into servo:master Oct 9, 2016
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
@bors-servo bors-servo mentioned this pull request Oct 9, 2016
Merged
4 of 4 tasks complete
@Manishearth Manishearth deleted the Manishearth:type-assert branch Oct 9, 2016
@bholley bholley mentioned this pull request Oct 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@Manishearth @highfive @bors-servo @emilio
You can’t perform that action at this time.