search

servo / rust-mozjs

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

Make Rooted do its job. #326

Closed Ms2ger closed 7 years ago

Ms2ger commented 7 years ago

This change is Reviewable

Ms2ger commented 7 years ago

My main fear is that the two out-of-line calls will be terrible for performance. I don't suppose we use Rooted terribly often…

Ms2ger commented 7 years ago

@jdm can you check the fixup commit? I'm not sure if the additional unsafety is worth it.

jdm commented 7 years ago

As mentioned on IRC, I think we should follow jonco's suggestion from https://github.com/servo/servo/issues/13096#issuecomment-250901349 rather than pursue the patch I wrote in that issue.

jdm commented 7 years ago

I'm glad to see the test, though!

jdm commented 7 years ago

@bors-servo: r+ Nice.

bors-servo commented 7 years ago

:pushpin: Commit c8f4cab has been approved by jdm

bors-servo commented 7 years ago

:hourglass: Testing commit c8f4cab with merge 568eccb...

bors-servo commented 7 years ago

:sunny: Test successful - status-appveyor, status-travis