Skip to content

Non-copy vector #14

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

Closed
rainyl opened this issue Dec 6, 2024 · 5 comments · Fixed by #16
Closed

Non-copy vector #14

rainyl opened this issue Dec 6, 2024 · 5 comments · Fixed by #16

Comments

@rainyl
Copy link
Owner

rainyl commented Dec 6, 2024

@Levi-Lesches
Copy link

Levi-Lesches commented Dec 6, 2024

Details: Modify all the vecXXX_cpp2c functions to pass the .data() and .size() directly. For example, this function:

inline VecPoint vecpoint_cpp2c(std::vector<cv::Point> v) {
auto* ptr = (CvPoint*)calloc(v.size(), sizeof(CvPoint));
for (int i = 0; i < v.size(); i++) {
ptr[i] = CvPoint{v[i].x, v[i].y};
}
return VecPoint{.ptr = ptr, .length = v.size()};
}

could be rewritten to be:

inline VecPoint vecpoint_cpp2c(std::vector<cv::Point> v) {
  return VecPoint {.ptr = v.data(), .length = v.size() };
}

Most important thing here is testing. Research shows that vector data should not be changed or overwritten or de-allocated so long as the vector is not modified. That means this would be very bad:

vector<cv::Point> vec;
// fill in vec here
auto result = vecpoint_cpp2c(vec);
vec.push(newPoint);

// THIS SHOULD NO LONGER BE USED!
// Adding new elements can change the internal memory structure of the vector,
// and invalidate the .data() pointer
return result; 

So the main hazard in this PR would be to guarantee that data is never changed after a _cpp2c() function is called.

@rainyl
Copy link
Owner Author

rainyl commented Dec 16, 2024

@Levi-Lesches How's it going? did you start working on this, if not I will submit a PR to work together.

@Levi-Lesches
Copy link

Nope, our winter break started today :)

Happy to start sometime soon but if you have the time go ahead

rainyl added a commit that referenced this issue Dec 18, 2024
@rainyl rainyl mentioned this issue Dec 18, 2024
@Levi-Lesches
Copy link

Awesome, thanks!

@rainyl
Copy link
Owner Author

rainyl commented Dec 19, 2024

😄, it works but still needs testing and optimizatios for production usage.

Fix it or let me know if you find any bugs.

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 a pull request may close this issue.

2 participants