servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Add lifetimes for Handle and MutableHandle #393

Closed marmistrz closed 6 years ago

marmistrz commented 6 years ago

Servo compiled fine for me with the following patch set.

Since the From trait consumes the object, I created a raw method for MutableHandle. This is not ideal, since into() would be more idiomatic. Is there anything better I could do or should I just leave it as it is?

There are two questions marked with // REVIEW since I'm not sure if the explicit lifetimes are the same what compiler inferred.

Closes #153.


This change is Reviewable

jdm commented 6 years ago

This will require some updates to the unit tests, too.

marmistrz commented 6 years ago

Honestly, I don't really have an idea what we could test here. That's just mimicking the Handle's behavior, all the checks are done by the compiler.

jdm commented 6 years ago

We could write some compile_fail doctests that demonstrate that handle values with lifetimes can't outlive the objects from which they were obtained.

bors-servo commented 6 years ago

:umbrella: The latest upstream changes (presumably #394) made this pull request unmergeable. Please resolve the merge conflicts.

jdm commented 6 years ago

Oh, and to be clear when I said "updates to the unit tests", I was specifically referring to the fact that they do not compile any longer according to TravisCI :)

jdm commented 6 years ago

The changes here look good to me as long as we can make Servo build with them before merging them.

bors-servo commented 6 years ago

:umbrella: The latest upstream changes (presumably #397) made this pull request unmergeable. Please resolve the merge conflicts.

marmistrz commented 6 years ago

@jdm Could some macros guru to review my macro? The macro rule duplication here is atrocious but I don't know if the macro system allows for anything more than copy paste.

nox commented 6 years ago

@marmistrz AFAICT skimming through that macro, the only way to require less clauses is to unconditionally transform all arguments, as opposed to just the ones with MutableHandle etc. This would require a trait implemented for all arguments found in invocations of wrap!. I think this is good enough for now.

bors-servo commented 6 years ago

:umbrella: The latest upstream changes (presumably #404) made this pull request unmergeable. Please resolve the merge conflicts.

jdm commented 6 years ago

Hmm, Unbox seems to be a platform-specific function for some reason:

error[E0425]: cannot find function `Unbox` in module `jsapi`
    --> src\rust.rs:1680:23
     |
1680 |         wrap!(@inner ($func_name ($($args)*) -> $outtype) <> () <> $($args)* ,);
     |                       ^^^^^^^^^^ not found in `jsapi`
     | 
    ::: src\jsapi_wrappers.in:3:1
     |
3    | wrap!(pub fn Unbox(cx: *mut JSContext, obj: HandleObject, vp: MutableHandleValue) -> bool);
     | ------------------------------------------------------------------------------------------- in this macro invocation
help: possible candidate is found in another module, you can import it into scope
     |
1693 |     use rust::wrappers::Unbox;
     |
jdm commented 6 years ago

@bors-servo r+

bors-servo commented 6 years ago

:pushpin: Commit 709357f has been approved by jdm

bors-servo commented 6 years ago

:hourglass: Testing commit 709357f7b07846380e3bc5dcbecb179884a4e023 with merge 03db1ae4b05a8b7c6e8da8bd3910a5d677f308da...

bors-servo commented 6 years ago

:sunny: Test successful - status-appveyor, status-travis Approved by: jdm Pushing 03db1ae4b05a8b7c6e8da8bd3910a5d677f308da to master...