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

Implement FromJSValConvertible for HandleValue #352

Merged
merged 1 commit into from Apr 25, 2017

Conversation

@KiChjang
Copy link
Member

KiChjang commented Apr 18, 2017

This change is Reviewable

@KiChjang
Copy link
Member Author

KiChjang commented Apr 18, 2017

r? @jdm or @nox

@KiChjang KiChjang force-pushed the KiChjang:convert-handlevalue branch from 6ba0700 to d038cd5 Apr 18, 2017
value: HandleValue,
_option: ())
-> Result<ConversionResult<HandleValue>, ()> {
Ok(ConversionResult::Success(value))

This comment has been minimized.

@nox

nox Apr 18, 2017

Member

value may be from another context, AFAIK this would be incorrect.

This comment has been minimized.

@jdm

jdm Apr 20, 2017

Member

We could add this:

unsafe {
    if value.is_object() {
        AssertSameCompartment(cx, value.to_object());
    }
}
@KiChjang KiChjang force-pushed the KiChjang:convert-handlevalue branch from d038cd5 to 5b57984 Apr 21, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 21, 2017

Assertion added.

@jdm
Copy link
Member

jdm commented Apr 21, 2017

error[E0425]: cannot find value `cx` in this scope
   --> src\conversions.rs:197:39
    |
197 |                 AssertSameCompartment(cx, value.to_object());
    |                                       ^^ did you mean `_cx`?
error: aborting due to previous error

Please be sure this builds :)

@KiChjang
Copy link
Member Author

KiChjang commented Apr 21, 2017

bah humbug

@KiChjang KiChjang force-pushed the KiChjang:convert-handlevalue branch from 5b57984 to e21cf29 Apr 21, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 21, 2017

'kay.

@jdm
Copy link
Member

jdm commented Apr 21, 2017

   Compiling mozjs_sys v0.0.0 (https://github.com/servo/mozjs#29132f67)
warning: unnecessary `unsafe` block
   --> src\conversions.rs:195:9
    |
195 |           unsafe {
    |  _________^ starting here...
196 | |             if value.is_object() {
197 | |                 AssertSameCompartment(cx, value.to_object());
198 | |             }
199 | |         }
    | |_________^ ...ending here: unnecessary `unsafe` block
    |
    = note: #[warn(unused_unsafe)] on by default
    Finished dev [unoptimized + debuginfo] target(s) in 622.32 secs
   Compiling js v0.1.4 (file:///C:/projects/rust-mozjs)
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
thread 'rustc' panicked at 'index out of bounds: the len is 8 but the index is 18446744073709551615', src\libcollections\vec.rs:1488
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: Could not compile `js`.
Build failed, waiting for other jobs to finish...
warning: unnecessary `unsafe` block
   --> src\conversions.rs:195:9
    |
195 |           unsafe {
    |  _________^ starting here...
196 | |             if value.is_object() {
197 | |                 AssertSameCompartment(cx, value.to_object());
198 | |             }
199 | |         }
    | |_________^ ...ending here: unnecessary `unsafe` block
    |
    = note: #[warn(unused_unsafe)] on by default
error: build failed

Unnerving. Fix the warning and let's see what happens?

@KiChjang KiChjang force-pushed the KiChjang:convert-handlevalue branch from e21cf29 to bfb109e Apr 21, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 21, 2017

omg, i don't know what's happening anymore, i just made a small change and rustc is so angry with me

@jdm
Copy link
Member

jdm commented Apr 23, 2017

Could you force push again? I'd like to see if this is an instance of rust-lang/rust#41415.

@jdm
Copy link
Member

jdm commented Apr 23, 2017

Nevermind, that's an actual assertion failure.

@jdm
Copy link
Member

jdm commented Apr 23, 2017

@KiChjang KiChjang force-pushed the KiChjang:convert-handlevalue branch 3 times, most recently from 7c9ea6d to 04e9e3a Apr 23, 2017
@jdm
Copy link
Member

jdm commented Apr 25, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2017

📌 Commit 04e9e3a has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2017

Testing commit 04e9e3a with merge 5ba3550...

bors-servo added a commit that referenced this pull request Apr 25, 2017
Implement FromJSValConvertible for HandleValue

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/352)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 5ba3550 to master...

@bors-servo bors-servo merged commit 04e9e3a into servo:master Apr 25, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@KiChjang KiChjang deleted the KiChjang:convert-handlevalue branch Apr 25, 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

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