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

impl<T> Read for UniquePtr<T> where ... Pin<&a mut T> : Read. #1368

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

anforowicz
Copy link
Contributor

This commit implements forwarding of Read trait implementation from UniquePtr<T> to the pointee type. This is quite similar to how Box<T> also forwards - see
https://doc.rust-lang.org/std/boxed/struct.Box.html#impl-Read-for-Box%3CR%3E

Just as with Box<T>, the impl cannot be provided in the crate introducing T, because this violates the orphan rule. This means that before this commit a wrapper newtype would be required to work around the orphan rule - e.g.:

```
struct UniquePtrOfReadTrait(cxx::UniquePtr<ffi::ReadTrait>);
impl Read for UniquePtrOfReadTrait { … }
```

After this commit, one can provide an impl that works more directly with the C++ type T (the FFI will typically require passing self: Pin<&mut ffi::ReadTrait>):

```
impl<'a> Read for Pin<&'a mut ffi::ReadTrait> { … }
```

For a more specific motivating example, please see: https://docs.google.com/document/d/1EPn1Ss-hfOC6Ki_B5CC6GA_UFnY3TmoDLCP2HjP7bms

@anforowicz
Copy link
Contributor Author

Can you PTAL?

Some notes:

  • I wasn't sure if additional tests were required here. It seems to me that if the new impl compiles, then any additional tests wouldn't really provide extra value (maybe other than verifying that this really works end-to-end; OTOH I've manually done this kind of verification by checking that http://review.skia.org/889876 works fine).
  • It may make sense to forward other std traits that Box<T> forwards, for example: Write, PartialEq, etc.
    • Please shout if you'd like me to:
      • Cover some additional traits in this PR (which traits?)
      • Cover some additional traits in follow-up PRs (which traits?)
    • I note that not all trait forwarding of Box<T> applies to cxx::UniquePtr<T>. For example:
      • From<T> takes T by value which is not possible for UniquePtrTargets (well, maybe except CxxString and CxxVector)

This commit implements forwarding of `Read` trait implementation from
`UniquePtr<T>` to the pointee type.  This is quite similar to how
`Box<T>` also forwards - see
https://doc.rust-lang.org/std/boxed/struct.Box.html#impl-Read-for-Box%3CR%3E

Just as with `Box<T>`, the `impl` cannot be provided in the crate
introducing `T`, because this violates the orphan rule.  This means that
before this commit a wrapper newtype would be required to work around
the orphan rule - e.g.:

    ```
    struct UniquePtrOfReadTrait(cxx::UniquePtr<ffi::ReadTrait>);
    impl Read for UniquePtrOfReadTrait { … }
    ```

After this commit, one can provide an `impl` that works more directly
with the C++ type `T` (the FFI will typically require passing `self:
Pin<&mut ffi::ReadTrait>`):

    ```
    impl<'a> Read for Pin<&'a mut ffi::ReadTrait> { … }
    ```

For a more specific motivating example, please see:
https://docs.google.com/document/d/1EPn1Ss-hfOC6Ki_B5CC6GA_UFnY3TmoDLCP2HjP7bms
@anforowicz anforowicz force-pushed the unique-ptr-blanker-trait-impls branch from 338d987 to c23c7ad Compare August 14, 2024 19:22
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

I am open to separate PRs for forwarding other traits (Write, PartialEq) as someone comes across a real use case for those.

@dtolnay dtolnay merged commit 6afd5ac into dtolnay:master Aug 15, 2024
16 checks passed
hubot pushed a commit to google/skia that referenced this pull request Aug 15, 2024
After dtolnay/cxx#1368 we can simplify the FFI
code a little bit, because now there is no need for a
separate newtype wrapper for `cxx::UniquePtr<ffi::ReadTrait>`.

Bug: chromium:359917956
Change-Id: Ib30413f45e0b41c54f87b35c78f6a65fda2015b3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/889876
Reviewed-by: Brian Osman <[email protected]>
Commit-Queue: Łukasz Anforowicz <[email protected]>
@anforowicz anforowicz deleted the unique-ptr-blanker-trait-impls branch September 19, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants