-
Notifications
You must be signed in to change notification settings - Fork 34
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
SHM mutation #328
Merged
yellowhatter
merged 11 commits into
eclipse-zenoh:main
from
ZettaScaleLabs:shm_mutation
Dec 18, 2024
Merged
SHM mutation #328
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
92fdcaf
- mutable access to SHM example
yellowhatter b95a926
code format
yellowhatter d13f2ee
Change all callbacks to give mutable Sample\Query|Reply
yellowhatter ee6c8ab
additional mutable accessors for query and reply contents
yellowhatter baeef99
make mutable access to sample/error for Reply private for a while
yellowhatter 55c8124
Merge commit '21b1d2cb391f8b6ac0420196c384d2d8d4ccc48a'
yellowhatter bb31c7c
code format
yellowhatter 9fad8b8
format
yellowhatter 9e1251a
code format
yellowhatter dbc7bbb
Update zenoh-c
yellowhatter d3099f4
fix compilation
yellowhatter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -433,14 +433,14 @@ class Session : public Owned<::z_owned_session_t> { | |
SubscriberOptions&& options = SubscriberOptions::create_default(), | ||
ZResult* err = nullptr) const { | ||
static_assert( | ||
std::is_invocable_r<void, C, const Sample&>::value, | ||
std::is_invocable_r<void, C, Sample&>::value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar fixes need to be done for every subscriber/queryable and get function :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
"on_sample should be callable with the following signature: void on_sample(zenoh::Sample& sample)"); | ||
static_assert(std::is_invocable_r<void, D>::value, | ||
"on_drop should be callable with the following signature: void on_drop()"); | ||
::z_owned_closure_sample_t c_closure; | ||
using Cval = std::remove_reference_t<C>; | ||
using Dval = std::remove_reference_t<D>; | ||
using ClosureType = typename detail::closures::Closure<Cval, Dval, void, const Sample&>; | ||
using ClosureType = typename detail::closures::Closure<Cval, Dval, void, Sample&>; | ||
auto closure = ClosureType::into_context(std::forward<C>(on_sample), std::forward<D>(on_drop)); | ||
::z_closure(&c_closure, detail::closures::_zenoh_on_sample_call, detail::closures::_zenoh_on_drop, closure); | ||
::z_subscriber_options_t opts; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
today there is no way to get mutable ReplyErr or Sample from reply, so this method is useless as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should add this. SHM buffer may reside literally everywhere where ZBytes is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the problem. We can not add these methods, because this would allow user to apply std::move to mutable Sample or ReplyError which would lead to their destruction. Since on zenoh-c level calling destructors is only possible on owned objects and sample/error are stored as loaned inside reply this would lead to UB.
So it is ok to merge this pr as is, but adding mutable access to sample/reply_err from Reply would require first resolving eclipse-zenoh/zenoh-c#718.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made mutable access to sample/reply_err from Reply private for a while.