Skip to content
This repository was archived by the owner on Nov 12, 2022. It is now read-only.

Implement FromJSValConvertible for HandleValue #352

Merged
merged 1 commit into from
Apr 25, 2017
Merged

Implement FromJSValConvertible for HandleValue #352

merged 1 commit into from
Apr 25, 2017

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Apr 18, 2017 •
edited by larsbergstrom
Loading

This change is Reviewable

@KiChjang
Copy link
Contributor Author

r? @jdm or @nox

@KiChjang KiChjang force-pushed the convert-handlevalue branch from 6ba0700 to d038cd5 Compare April 18, 2017 17:22
nox
nox reviewed Apr 18, 2017
View reviewed changes
src/conversions.rs Outdated
value: HandleValue,
_option: ())
-> Result<ConversionResult<HandleValue>, ()> {
Ok(ConversionResult::Success(value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add this:

unsafe {
    if value.is_object() {
        AssertSameCompartment(cx, value.to_object());
    }
}

@KiChjang KiChjang force-pushed the convert-handlevalue branch from d038cd5 to 5b57984 Compare April 21, 2017 04:46
@KiChjang
Copy link
Contributor Author

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
Contributor Author

bah humbug

@KiChjang KiChjang force-pushed the convert-handlevalue branch from 5b57984 to e21cf29 Compare April 21, 2017 05:19
@KiChjang
Copy link
Contributor Author

'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 convert-handlevalue branch from e21cf29 to bfb109e Compare April 21, 2017 23:00
@KiChjang
Copy link
Contributor Author

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

Filed rust-lang/rust#41484.

@KiChjang KiChjang force-pushed the convert-handlevalue branch 3 times, most recently from 7c9ea6d to 04e9e3a Compare April 25, 2017 05:38
@jdm
Copy link
Member

jdm commented Apr 25, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 04e9e3a has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 04e9e3a with merge 5ba3550...

bors-servo pushed a commit that referenced this pull request Apr 25, 2017
Auto merge of #352 - KiChjang:convert-handlevalue, r=jdm
5ba3550 
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

☀️ 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
@KiChjang KiChjang deleted the convert-handlevalue branch April 25, 2017 06:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jdm jdm jdm left review comments

@nox nox nox left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KiChjang @jdm @bors-servo @nox