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

Introduce new macro to wrap a C++ class: cpp_class! #28

Merged
merged 8 commits into from
Jun 7, 2018
Merged

Introduce new macro to wrap a C++ class: cpp_class! #28

merged 8 commits into from
Jun 7, 2018

Conversation

ogoffart
Copy link
Collaborator

A macro which allow to wrap C++ class into rust types with and call
their destructor and copy constructor appropriatly

Example of use:

cpp_class!(pub struct MyClass, "MyClass");
impl MyClass {
    fn new() -> Self {
        unsafe { cpp!([] -> MyClass as "MyClass" { return MyClass(); }) }
    }
    fn member_function(&self, param : i32) -> i32 {
        unsafe { cpp!([self as "const MyClass*", param as "int"] -> i32 as "int" {
            return self->member_function(param);
        }) }
    }
}

A macro which allow to wrap C++ class into rust types with and call
their destructor and copy constructor appropriatly
@ogoffart
Copy link
Collaborator Author

@mystor Do you have any feedback to give?

I'm slowly developing a crate that uses a C++ library, and uses this feature and the other ones.
Would be nice to have it merged.

@ogoffart
Copy link
Collaborator Author

@mystor ping?

@mystor
Copy link
Owner

mystor commented Apr 16, 2018

@ogoffart Hey, so sorry for how long this has taken. I'll try to take a look at this in the next few days, but I'm pretty busy with work right now.

Copy link
Owner

@mystor mystor left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thank you so much for doing this work & sorry for how long it's taken me to get back to you. I'll try to be more prompt with follow-ups.

cpp/src/lib.rs Outdated
/// class is copyable (or Copy if it is trivialy copyable), and Default if the class
/// is default constructible
///
/// Warning: This only work if the C++ class can be moved in memory (using
Copy link
Owner

Choose a reason for hiding this comment

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

nit: Can you make this warning more prominent (perhaps above the example code invocation?). I think it's pretty important :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's pretty important and quite a bad restriction.
There is unfortunately afaik no way to tell the rust compiler that a type should not be moved around, but only copied, or to call the move constructor.

Fortunately, most value types i've been using can be safely moved. Altough this is not the case for types from the standard library such as std::string.

cpp/src/lib.rs Outdated
/// is default constructible
///
/// Warning: This only work if the C++ class can be moved in memory (using
/// memcpy). This disallow most classes from the standard library.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: s/disallow/disallows

cpp/src/lib.rs Outdated
///
#[macro_export]
macro_rules! cpp_class {
(struct $name:ident, $type:expr) => {
Copy link
Owner

Choose a reason for hiding this comment

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

It might be really slick to support this as a mode of operation for the cpp! macro (e.g. cpp! { struct... } or cpp! { pub struct... }), rather than introducing the cpp_class macro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats a good idea.

cpp! { struct Foo as "Foo" }

Note that is was thinking the macro could be also extended to more complex constructs

// This will automatically use operator==
cpp! { #[derive(Eq)] struct Foo as "Foo" }

// map the members to the C++ structure and somehow check it is valid
cpp! { struct Point as "Point" {  x : u32,   y: u32  }  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, i'm a bit unsure.

having only one macro cpp! could be nice, and would avoid wondering if cpp_class is the right name or if it should be cpp_struct.

However, having a separate macro makes it easier to document or to lookup what it is, than having a single macro that does everything.

hash = hash
));
} else {
sizealign.push(format!("{{
{hash}ull,
sizeof({type}),
rustcpp::AlignOf<{type}>::value
rustcpp::AlignOf<{type}>::value,
rustcpp::Flags<{type}>::value
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps this could be made cleaner by unifying all of this logic into a single ::Flags-like invocation. We could just have a list of rustcpp::Meta<{type}, {hash}ull>::value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean.
You mean that rustcpp::Meta<{type}, {hash}ull>::value would be a SizeAlign object?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that was my thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs C++11's constexpr, so it depends on the other change that enables C++11

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that MSVC 2013 which is still tested in the CI doesn't support constexpr enough to get this to work.

extern "C" {{
void __cpp_destructor_{hash}(void *ptr) {{
typedef {cpp_name} T;
static_cast< {cpp_name} *>(ptr)->~T();
Copy link
Owner

Choose a reason for hiding this comment

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

Why use static_cast<{cpp_name} *> here instead of just casting to T*?

}


Copy link
Owner

Choose a reason for hiding this comment

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

nit: extra whitespace

test/src/lib.rs Outdated

#[test]
fn move_only() {
cpp_class!(struct MoveOnly, "MoveOnly");
Copy link
Owner

Choose a reason for hiding this comment

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

nit: perhaps use as instead of the comma in cpp_class! to make it more consistent with how the type is usually used?

output,
r#"
extern "C" {{
void __cpp_destructor_{hash}(void *ptr) {{
Copy link
Owner

Choose a reason for hiding this comment

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

It's unfortunate that we define these no-op functions even if we will never use them (because they aren't available). :-/ Not much that can be done about that AFAIK though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the destructor and copy constructor, we can't do anything.

For the default constructor, I was considering making it explicit by having to write
#[derive(Default)] in the macro.

I am actually undecided if it is good to have it explicit or implicit.

Implicit is quite convinient : if the C++ has a default constructor, it is automatic. But the rust rules seems to prefer explicit for this kinf of things.

But then what about all the other trait, should we automatically derive PartialEq or PartialOrd if the operator exists?

typename enable_if<is_copy_constructible<T>::value>::type copy_helper(const void *src, void *dest)
{ new (dest) T (*static_cast<T const*>(src)); }
template<typename T>
typename enable_if<!is_copy_constructible<T>::value>::type copy_helper(const void *, void *) { }
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps make this code path abort() the process, just to double-down on the "this should never be called" angle?

@@ -45,6 +45,50 @@ const INTERNAL_CPP_STRUCTS: &'static str = r#"
#include "stdint.h" // For {u}intN_t
#include <new> // For placement new

#if __cplusplus <= 199711L && !defined(_MSC_VER)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we support C++ < 11 right now.

@ogoffart
Copy link
Collaborator Author

I made most change, but I did not rename the macro to re-use cpp.

I think it is better to be implicit about the difference of usage of the macro.
That makes the code more readable and lets the user find the documentation for the relevant macro more easily. Is this also not a reason why there are no overloaded functions in rust?
This is not a strong opinion. What do you think?

For example if there is a typo in the C++ class name, or if the class
cannot be destructed or is only forward declared.
Note that some type traits are still not implemented in old GCC
@mystor
Copy link
Owner

mystor commented Jun 7, 2018

Thanks for the changes :-) Looks good now.

@mystor mystor merged commit e727d74 into mystor:master Jun 7, 2018
mystor pushed a commit that referenced this pull request Jun 7, 2018
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.

None yet

2 participants