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

Remove Option wrappers for owned objects #925

Open
milyin opened this issue Feb 20, 2025 · 0 comments
Open

Remove Option wrappers for owned objects #925

milyin opened this issue Feb 20, 2025 · 0 comments

Comments

@milyin
Copy link
Contributor

milyin commented Feb 20, 2025

Describe the feature

The Rust object is represented in zenoh-c's owned structure is one of the following ways:

  • as Option. In this case gravestone state (state after drop) is none
  • just as RustObject. Gravestone object is some empty state of the object.

It would be good to get rid of first representation by providing internal "gravestone" state to all rust objects used in zenoh-c.
This would allow to avoid potential crashes when zenoh-cpp attempts to perform "std::move" on some mutable property of rust object.

This is already implemented for some objects, like payload: the C and C++ code are allowed to "take" payload from mutable reference to payload, replacing existing payload with "empty" one (with null data pointer and zero size).

But if the e.g. Sample is available by mutable reference, we can't simply take it, because it's represented in C++ as Option<Sample> and we'll put value None to &mut Sample, which will cause crash.

At this moment the problem is solved by operation take_from_loaned which is implemented only for objects which needs this functionality (callback parameters) and replaces original Sample,Query,etc with empty one. This "empty" value is not a real "gravestone" state, i.e. it may require a drop. See PR #923

But the better solution proposed by @DenisBiryukov91 is to provide such "empty" states for all objects and make them "gravestone", i.e. use them in drop. This will improve code simplicity (no extra "Option" wrapper) and safety (when object is available by mutable reference, it's aways safe to take it, replacing with empty state)

@milyin milyin changed the title Remove Option wrappers in for owned objects Remove Option wrappers for owned objects Feb 21, 2025
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

No branches or pull requests

1 participant