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

Add as_void_ptr helper method to &T #15252

Closed
jdm opened this issue Jan 26, 2017 · 9 comments
Closed

Add as_void_ptr helper method to &T #15252

jdm opened this issue Jan 26, 2017 · 9 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 26, 2017

There are lots of places where we write code like &foo *const _ as *const _ (like here, here, and here) where the receiver code expects a *const c_void. This code would be clearer if we could use foo.as_void_ptr() where as_void_ptr is a trait method that's implemented for every type T.

@jdm jdm added the I-cleanup label Jan 26, 2017
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 26, 2017

Would the T in this case be T: DomStruct? I don't think we can globally define methods for every type yet...

@jdm
Copy link
Member Author

@jdm jdm commented Jan 26, 2017

We can define a trait and import it everywhere that it's needed.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 26, 2017

Ah, I see what this is. Create a trait:

pub trait AsVoidPtr {
    fn as_void_ptr(&self) -> *const c_void
}

And then implementing this trait on every foo struct where we use the &foo as *const _ as *const _ pattern.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 26, 2017

I'm pretty sure we can implement the trait for a generic type T, rather than specific implementations for types we care about.

@jdm jdm added the E-easy label Mar 16, 2017
@highfive
Copy link

@highfive highfive commented Mar 16, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@mckaymatt
Copy link
Contributor

@mckaymatt mckaymatt commented Mar 29, 2017

@highfive: assign me

@highfive highfive added the C-assigned label Mar 29, 2017
@highfive
Copy link

@highfive highfive commented Mar 29, 2017

Hey @mckaymatt! Thanks for your interest in working on this issue. It's now assigned to you!

@mckaymatt
Copy link
Contributor

@mckaymatt mckaymatt commented Mar 31, 2017

@jdm Is this what you are talking about?

use std::os::raw::c_void;

trait AsVoidPtr {
    fn as_void_ptr(&self) -> *const c_void;
}

impl<T> AsVoidPtr for T {
    fn as_void_ptr(&self) -> *const c_void {
        // &*self as *const _ as *const c_void
        &*self as *const _ as *const c_void
    }
}

I'm thinking I would put it someplace like components/script/lib.rs? Or maybe it's own crate components/pointer. Or components/script/dom/bindings?

@jdm
Copy link
Member Author

@jdm jdm commented Mar 31, 2017

dom/bindings/utils.rs would make sense to me. You should not need the &* in &*self, though, and it couldn't hurt to replace the _ with T.

@mckaymatt mckaymatt mentioned this issue Apr 3, 2017
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Apr 4, 2017
Add as_void_ptr helper method to &T

<!-- Please describe your changes on the following line: -->
r? @jdm
issue #15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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/16234)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 5, 2017
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? @jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : d5992625f30523ee372f54d0005b211d6c9a6b41
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue Apr 11, 2017
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? @jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Apr 12, 2017
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? @jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df

UltraBlame original commit: 7186df029be0a17d5e97806e6ea9e923b2a5082c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df

UltraBlame original commit: 7186df029be0a17d5e97806e6ea9e923b2a5082c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…matt:as_void_ptr_helper_method_15252); r=jdm

<!-- Please describe your changes on the following line: -->
r? jdm
issue servo/servo#15252
The primary goal of this PR is to add add a generic trait method that returns a void ptr.

In addition to that change, I made the casting explicit in `components/script/dom/bindings/callback.rs`  and `components/script/dom/promise.rs`. I did not use the new trait method because `AddRawValueRoot` is not looking for a `c_void`. It's looking for `std::os::raw::c_char`.
```rust
pub fn AddRawValueRoot(cx: *mut JSContext, vp: *mut Value,
                                          name: *const ::std::os::raw::c_char) -> bool;
```
So I replace the `as *const _ ` with a more specific cast.

---
<!-- 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
- [x] These changes fix #15252

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because
 This seems like code cleanup. It shouldn't change behaviour.

<!-- 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: 8d8fea0b4bf323be42eff3ad5624ce33892fb6df

UltraBlame original commit: 7186df029be0a17d5e97806e6ea9e923b2a5082c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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