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 UniquePtrTarget for T; #329

Closed
wants to merge 1 commit into from

Conversation

adetaylor
Copy link
Collaborator

Builds on top of #325. Fixes #236.

@sbrocket
Copy link
Contributor

sbrocket commented Oct 2, 2020

Can we do this without exposing - and requiring users to know - the internal trait names like UniquePtrTarget? In #308 dtolnay@ suggested something like impl Vec<T> {} and impl UniquePtr<T> {}.

@adetaylor
Copy link
Collaborator Author

Yes, probably, I will amend that way after I eventually rebase on top of your stuff.

@sbrocket
Copy link
Contributor

sbrocket commented Oct 2, 2020

One other thing to consider here. For Vec and Box after my PR #308, the following only fails at C++ compile time because we don't have traits similar to UniquePtrTarget and VectorElement for those that can be checked with a trait bound.

#[cxx::bridge]
mod here {
    struct Shared {
        z: usize,
    }
}

#[cxx::bridge]
mod there {
    type Shared = crate::here::Shared;

    extern "C" {
        fn c_take_rust_vec_shared(v: Vec<Shared>);
    }
}

fn main() {}

We might want to add those in a separate PR to improve the diagnostics, plus then we can point people at this option.

cxx allows UniquePtr<T> only if T implements a trait,
UniquePtrTarget. This trait implementation is automatically generated
for type T only if:

* UniquePtr<T> is used in a struct or a function parameter/return
  type; AND
* the type T is a C++ type.

Until now, it was not possible to generate UniquePtrTarget
implementations for type aliases where a cxx type refers to an
externally defined type (e.g. from bindgen).

There were also legitimate uses for UniquePtr<T> even if none
of the structs/functions within the cxx::bridge actually used it;
for example storing a UniquePtr<T> in some other struct outside
the cxx::bridge mod.

This change allows any cxx::bridge block to contain
  impl UniquePtrTarget for T {}
which will force cxx to generate the UniquePtrTarget trait (and
associated C++ code) for that type.

It's not desirable to do this for _every_ type since it does
generate additional C++ code as well as Rust code, and thus in some
toolchains may cause binary bloat.

Future changes could allow similar syntax for types which should
be containable in a C++ vector, e.g.:
  impl VectorElement for T {}
but that's not yet done here.
@dtolnay dtolnay marked this pull request as ready for review October 4, 2020 04:33
@dtolnay dtolnay closed this in #336 Oct 4, 2020
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 for the PR. :) I ended up merging #336 instead, which follows the approach from #312 (comment) of using impl UniquePtr<T> {} for a requested instantiation. As @sbrocket points out, this is more closely connected to the public API of the cxx crate and we avoid making the user learn a second set of names for the internal names of the traits.

@adetaylor
Copy link
Collaborator Author

Sounds good. Thank you!

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.

Legitimate uses for UniquePtrs even if not currently expanded
3 participants