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

C++ std::vector<T> and Rust std::vec::Vec<T> support #67

Merged
merged 3 commits into from
Apr 26, 2020
Merged

C++ std::vector<T> and Rust std::vec::Vec<T> support #67

merged 3 commits into from
Apr 26, 2020

Conversation

myronahn
Copy link
Contributor

Implements #33.

Add basic std::vector and std::vec::Vec support across FFI boundary.

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, this looks great!

My two higher priorities at the moment are Result support and function pointer support. Once those are in better shape, I'll be able to work on reviewing and merging vector support.

@myronahn
Copy link
Contributor Author

Thanks, I appreciate the response and I know you're busy.

Looking forward to Result and function pointer support as well.

@dtolnay
Copy link
Owner

dtolnay commented Mar 30, 2020

Update: I've finished what I wanted on Result support and function pointer support so I am going to start looking at this next.

@myronahn
Copy link
Contributor Author

Much appreciated. In the meanwhile I'll work on keeping the branch current with the latest changes.

@dtolnay
Copy link
Owner

dtolnay commented Apr 5, 2020

Any idea why this might be breaking the Windows builds in Travis (both gnu and msvc)?

error LNK2019: unresolved external symbol cxxbridge02$std$vector$u16$get_unchecked referenced in function _ZN60_$LT$u16$u20$as$u20$cxx..vector..VectorTarget$LT$u16$GT$$GT$13get_unchecked17h637ab12787a55ed7E
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u16$length referenced in function _ZN60_$LT$u16$u20$as$u20$cxx..vector..VectorTarget$LT$u16$GT$$GT$13vector_length17h94e71af891c12ecaE
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u16$push_back referenced in function _ZN60_$LT$u16$u20$as$u20$cxx..vector..VectorTarget$LT$u16$GT$$GT$9push_back17h1e82a956b23b9d19E
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u32$get_unchecked referenced in function _ZN60_$LT$u32$u20$as$u20$cxx..vector..VectorTarget$LT$u32$GT$$GT$13get_unchecked17hd66a7b7eb520643bE
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u32$length referenced in function _ZN60_$LT$u32$u20$as$u20$cxx..vector..VectorTarget$LT$u32$GT$$GT$13vector_length17h02cd8ef504a64da5E
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u32$push_back referenced in function _ZN60_$LT$u32$u20$as$u20$cxx..vector..VectorTarget$LT$u32$GT$$GT$9push_back17h7fd3172a49834d2cE
error LNK2019: unresolved external symbol cxxbridge02$std$vector$u64$get_unchecked referenced in function _ZN60_$LT$u64$u20$as$u20$cxx..vector..VectorTarget$LT$u64$GT$$GT$13get_unchecked17h554a62cb7365e7bdE
...

@myronahn
Copy link
Contributor Author

myronahn commented Apr 5, 2020

I'm pretty sure I know why it is failing. I pre-generate all the vector code for all the primitive types on the Rust side but only generate what is used on the C++ side. This worked fine on Mac/Linux but the Windows linker seems to be more strict. I'll fix it.

@myronahn
Copy link
Contributor Author

myronahn commented Apr 6, 2020

Okay it should be working now.

@dtolnay
Copy link
Owner

dtolnay commented Apr 6, 2020

Thanks -- this fix violates ODR though right? If one program contains two different cxx::bridge modules and they each define cxxbridge02$std$vector$u8$length etc, then that is more than one definition (and having the same code doesn't make it not UB).

@myronahn
Copy link
Contributor Author

myronahn commented Apr 6, 2020

Good point, I didn't think of that.

I suppose moving the implementations for all the primitive types to cxx.cc would make the most sense.

@myronahn
Copy link
Contributor Author

myronahn commented Apr 6, 2020

Moved implementations for the vector methods and the unique_ptr<vector> methods to cxx.cc for primitive types.

@dtolnay dtolnay merged commit 507c2d7 into dtolnay:master Apr 26, 2020
@dtolnay dtolnay mentioned this pull request Apr 26, 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.

I squashed this to avoid bringing the nonexistent "alpha" release commits into the repo, but the original tree is available at 85d5622. I merged this with a number of fixes in #148. The diff of the two PRs together is e9f58d5...1768d8f.

@myronahn
Copy link
Contributor Author

David, huge appreciation for merging this and huge guilt for the amount of cleanup you had to do!

For future reference, if you had said, "It's easier for me to reimplement this," or "Please clean some of your code up," my ego can take the hit no problem.

Thanks again and I'll work on some of the subsequently generated issues.

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