Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd safe Heap::boxed constructor #395
Merged
Conversation
|
I agree. This is a useful API to provide. |
|
|
bors-servo
added a commit
that referenced
this pull request
Mar 12, 2018
Add safe Heap::boxed constructor While this doesn't remove old Heap constructor (#343), it adds a new `boxed` function along with some additional explanation while this may be preferred until a better/different solution comes up to construct a Heap value directly with an appropriate value set, in a safe manner. This made my work (which I'll upload later) on typed arrays, and also indirectly for objects, in webidl unions safer and more convenient, so I think it's a good addition. r? @jdm <!-- 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/395) <!-- Reviewable:end -->
|
|
bors-servo
added a commit
to servo/servo
that referenced
this pull request
Mar 14, 2018
Fix JS object conversion in unions <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? @jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/20265) <!-- Reviewable:end -->
bors-servo
added a commit
to servo/servo
that referenced
this pull request
Mar 14, 2018
Fix JS object conversion in unions <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? @jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/20265) <!-- Reviewable:end -->
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Mar 14, 2018
…k:fix-js-objects-in-unions); r=jdm <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? @jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e597cd9e1adc23ae30587ff6bffc05119ac33fb9 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : b38e013f207e5abf4b18fd57055a5c9713cf85c2
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Oct 2, 2019
…k:fix-js-objects-in-unions); r=jdm <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e597cd9e1adc23ae30587ff6bffc05119ac33fb9 UltraBlame original commit: 9ae60e8e7b9f37299ca1be4f3558f0cea18d13cd
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this pull request
Oct 2, 2019
…k:fix-js-objects-in-unions); r=jdm <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e597cd9e1adc23ae30587ff6bffc05119ac33fb9 UltraBlame original commit: 9ae60e8e7b9f37299ca1be4f3558f0cea18d13cd
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Oct 2, 2019
…k:fix-js-objects-in-unions); r=jdm <!-- Please describe your changes on the following line: --> Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e597cd9e1adc23ae30587ff6bffc05119ac33fb9 UltraBlame original commit: 9ae60e8e7b9f37299ca1be4f3558f0cea18d13cd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Xanewok commentedMar 10, 2018
•
edited
While this doesn't remove old Heap constructor (#343), it adds a new
boxedfunction along with some additional explanation while this may be preferred until a better/different solution comes up to construct a Heap value directly with an appropriate value set, in a safe manner.This made my work (which I'll upload later) on typed arrays, and also indirectly for objects, in webidl unions safer and more convenient, so I think it's a good addition.
r? @jdm
This change is