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

Translate C++ constructors to new(...) #221

Closed
abrown opened this issue Jun 18, 2020 · 7 comments
Closed

Translate C++ constructors to new(...) #221

abrown opened this issue Jun 18, 2020 · 7 comments

Comments

@abrown
Copy link

abrown commented Jun 18, 2020

I have only briefly looked into cxx (thanks for the library, BTW!) and I was wondering how to describe access to C++ constructors (and other class methods, for that matter) from Rust (using cxx::bridge). Is this possible and, if so, could you describe a simple example?

If it is currently not implemented, then is it still possible? It looks to me like the cxx::bridge macro only currently understands C++ functions--how would I describe constructors and methods contained within a type?

@abrown
Copy link
Author

abrown commented Jun 18, 2020

Oh, and to explain the title: should C++ constructors be mapped to the more conventional T::new(...) in Rust?

@dtolnay
Copy link
Owner

dtolnay commented Jun 18, 2020

To the extent that constructors "return" a C++ type by value, they're out of scope for this library because Rust moves (memcpy) are incompatible with C++ moves (which require a constructor to be called). Translating a constructor to fn new() -> Self would not be correct.

You can bind them unsafely with bindgen which assumes moving without a constructor call is okay, or you can use the "shared struct" approach in the readme which is safely movable in either language, or you can include! a shim which does the construction behind a unique_ptr or similar.

At my work we do this kind of thing:

// ZeusClientWrapper.cpp

std::unique_ptr<ZeusClient> zeus_client_new(
    rust::Str clientId,
    rust::Str smcTier) {
  return std::make_unique<ZeusClient>(
      zeus::client::getClientForSMCTier(
          std::string(clientId), std::string(smcTier)));
}
// zeus_cpp_client.rs

pub struct ZeusCppClient {
    client: UniquePtr<ffi::ZeusClient>,
}

impl ZeusCppClient {
    pub fn new(client_id: &str, smc_tier: &str) -> Result<ZeusCppClient> {
        ffi::zeus_client_new(client_id, smc_tier)
            .map(|client| ZeusCppClient { client })
    }
}

Regarding non-constructor static member functions, it's waiting on me to review and iterate on #220 but will look something like (not fully designed yet):

// class Klass {
// public:
//   static void staticMethod();
// };

#[cxx::bridge]
mod ffi {
    extern "C" {
        type Klass;

        // becomes ffi::Klass::staticMethod in Rust
        #[class = "Klass"]
        fn staticMethod();

        // or becomes ffi::staticMethod in Rust
        #[foreign_name = "Klass::staticMethod"]
        fn staticMethod();
    }

    #[class = "Klass"]
    extern "C" {
        // or like this if you have lots on the same class
        fn staticMethod();
    }
}

@abrown
Copy link
Author

abrown commented Jun 18, 2020

Thanks! That makes a lot of sense. I had balked at the wrapper idea since it seemed like a bunch of work but it may not be that bad, especially since it handles the object lifetime correctly.

What about non-static methods? (This may seem obvious to you but I want to check). Before #220 is merged, would it look something like the following?

#[cxx::bridge]
mod ffi {
    extern "C" {
        type Klass;

        // if in C++ we defined Klass::classMethod, would we describe it with the following:
        fn classMethod(this: &mut Klass, parameter: ...);
        // and then use it with an instantiated klassObject like klassObject.classMethod(parameter) in Rust
    }
}

@dtolnay
Copy link
Owner

dtolnay commented Jun 18, 2020

Non-static member functions already work in both directions as of cxx 0.2.10 (implemented in #121). We would only need #220 for static ones.

Pretty much as you said:

// class Klass {
// public:
//   void mutMethod();
//   void constMethod() const;
// };

#[cxx::bridge]
mod ffi {
    extern "C" {
        type Klass;

        fn mutMethod(&mut self);
        fn constMethod(&self);

        // or to disambiguate if the enclosing `extern "C"` block has more
        // than one `type`:
        fn constMethod(self: &Klass);
    }
}

Next big thing on my plate is writing a guide... #179.

@dtolnay
Copy link
Owner

dtolnay commented Jun 18, 2020

I had balked at the wrapper idea since it seemed like a bunch of work but it may not be that bad, especially since it handles the object lifetime correctly.

That's my assessment too. We can consider adding sugar there down the line if there are truly repetitive patterns.

The following is what we had before, based on bindgen, which was more work and unsafe all the way through. (Just about every line on the C++ side as well as the Rust side could be a memory safety bug if we aren't paying attention.)

// ZeusClientWrapper.cpp

ZeusClient* FOLLY_NULLABLE zeus_client_new(
    folly::StringPiece clientId,
    folly::StringPiece smcTier,
    std::string** FOLLY_NONNULL exn) noexcept {
  CHECK_NONNULL(exn);

  ZeusClient* res = nullptr;
  *exn = catch_exception([&]() {
    auto wrapper = std::make_unique<ZeusClient>();
    wrapper->client = zeus::client::getClientForSMCTier(
        clientId.str(), smcTier.str());
    res = wrapper.release();
  });

  return res;
}

void zeus_client_free(ZeusClient* wrapper) noexcept {
  delete wrapper;
}
// zeus_cpp_client.rs

pub struct ZeusCppClient {
    client: NonNull<ffi::ZeusClient>,
}

impl Drop for ZeusCppClient {
    fn drop(&mut self) {
        unsafe {
            ffi::zeus_client_free(self.client.as_ptr());
        }
    }
}

impl ZeusCppClient {
    pub fn new(client_id: &str, smc_tier: &str) -> Result<ZeusCppClient> {
        catch!(unsafe {
            ffi::zeus_client_new(
                *StringPiece::from(client_id).as_inner(),
                *StringPiece::from(smc_tier).as_inner(),
            )
        })
        .map(|ptr| ZeusCppClient {
            client: NonNull::new(ptr).unwrap(),
        })
    }
}

@abrown
Copy link
Author

abrown commented Jun 19, 2020

Next big thing on my plate is writing a guide... #179.

Looks like we are well on our way with this issue 😆. Thanks for your help!

@abrown abrown closed this as completed Jun 19, 2020
@abrown abrown mentioned this issue Jul 9, 2020
@dtolnay
Copy link
Owner

dtolnay commented Sep 4, 2020

I filed #280 with a design for the special case of types which we can statically verify do not have a move constructor.

Repository owner locked and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants