-
Notifications
You must be signed in to change notification settings - Fork 370
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
rerun::Collection
borrows data too eagerly, making it very easy to cause segfaults & read of invalid data
#7081
Comments
This is pretty much by design and document as such (although maybe not easy to find :/). That said, I'm starting to regret this design. It's great to have the option of borrowing data, but we do it too aggressively. This isn't Rust where we can do this kind of operation safely, instead this makes everything very easy to fail as you've experienced. Somewhat related to this recent change here where I made it easier to do explicit borrow & ownership taking. |
rerun::Collection
borrows data too eagerly, making it very easy to cause segfaults & read of invalid data
Yes, I can understand that you are trying to import good ideas from Rust, but without the compiler support, you CANNOT do that. My suggestion is to make the C++ API more idiomatic, using a more idiomatic C++ API. I suggest adding these constructors // zero-cost move semantic. Probably what I would propose by default in the tutorials
Collection(std::vector<T>&& vect)
// Reference to extenal object. Everyone understands that Collection is NOT taking ownership of this object
// and that the caller is responsible for its life-time.
// Looks unsafe, AS IT SHOULD
Collection(const std::vector<T>* vect)
// The only safe option is to make a copy. No borrowing
Collection(const std::vector<T>& vect); |
I'm not entirely sold on the idea that passing a pointer signifies borrowing data of the underlying vector well enough. Since rerun collection takes internally a pointer to Also note that these two Collection(std::vector<T>&& vect);
Collection(const std::vector<T>& vect); are more or less redundant to each other an can be replaced with
which then internally always moves.
We use that pattern in a lot of places already to cull down on the number of constructors/builder methods. |
For posterity and in defense of how The idea of "adapt data on the fly without copy" is the reason |
Yes, this is why I started creating this helper functions to make the code nicer 😉 You are correct about the fact that The constructor
Are intentionally "ugly" because most C++ developer will look at that and a big red flag will light in their brain. That is specifically what I want 😄 |
The "pointer-means-borrowing is a neat idea, but it leaves the problem of a user now being able to pass in |
Describe the bug
I have a class like this:
Unfortunately, the result seems to be corrupted, ore specifically the centers (half-sizes, seems to be ok).
I believe that the problem is that the
Collection
is borrowing the reference to the centers, instead of taking owership.The following code fixes the issue:
To Reproduce
Try the function mentioned above and then print the centers:
Rerun version
0.17.0
The text was updated successfully, but these errors were encountered: