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

Support pod extern_cpp_types #1193

Merged
merged 4 commits into from
Jan 30, 2023
Merged

Support pod extern_cpp_types #1193

merged 4 commits into from
Jan 30, 2023

Conversation

adetaylor
Copy link
Collaborator

Test for #1192

We now ensure that all extern_cpp_types are ingested by the ByValueChecker
before it considers any structs.
quote! {

extern_cpp_type!("VecThingy", crate::VecThingy)
pod!("VecThingy")

Choose a reason for hiding this comment

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

Thinking about this a bit more: maybe this test should actually be failing, unless the target Rust type implements Copy? IIUC, that's basically what pod! expects?

Choose a reason for hiding this comment

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

(as in, my example from the original issue should probably also fail because I forgot #[derive(Clopy, Copy)] which is the only guarantee that the type is, in fact, trivial)

Choose a reason for hiding this comment

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

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, no time for autocxx at the moment - I will reply next time I get back to the project. (However your mental model of how pod! works is wrong: I need to spend some time documenting exactly what are the expectations are for extern_cpp_type! users in the POD and non-POD case.)

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 improved the documentation here in #1221.

TL;DR: deriving things in Rust is not material to whether things can be POD or non-POD; it's all about the nature of the pre-existing C++ constructors/destructors, and by the time we get as far as #[derive] there's no way we can add extra C++ assertions.

Overall, this extern_cpp_type facility was intended primarily for one set of autocxx bindings to refer to a type generated by some other autocxx/cxx bindings, i.e. cross-mod links, rather than for folks such as your good self to generate whole new fresh shiny Rust types. I've clarified that too.

@RReverser
Copy link

For now added this to my project via { version = "0.23.1", git = "https://github.com/google/autocxx", rev = "bd0efed" } and can confirm this fixes my first immediate issue, thanks.

@RReverser
Copy link

Two questions. For example, I have a struct like this:

struct RansacStats {
    size_t refinements = 0;
    size_t iterations = 0;
    size_t num_inliers = 0;
    double inlier_ratio = 0;
    double model_score = std::numeric_limits<double>::max();
};

I generate it with generate! + pod!, and it works great. But:

  1. Why does the generated binding have Drop? Probably related to my misunderstanding of what pod! means, but it seems clearly unnecessary in this case?
  2. Is it possible to have Default generated for such struct, where all fields actually have default initializers on the C++ side?

@RReverser
Copy link

Wait... now I see at least generated fn new for this struct. Not sure what changed between reruns.

@RReverser
Copy link

On some classes it still doesn't exist, but that sounds like #1155.

@adetaylor adetaylor changed the title Add test for issue 1192. Support pod extern_cpp_types Jan 30, 2023
@adetaylor adetaylor merged commit 9e69fae into main Jan 30, 2023
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