-
Notifications
You must be signed in to change notification settings - Fork 116
[#1312] no zero copy send for option result #1313
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
[#1312] no zero copy send for option result #1313
Conversation
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.
Pull request overview
This PR addresses issue #1312 by removing Option<T> and Duration from the default ZeroCopySend trait implementations and introducing zero-copy safe alternatives: StaticOption<T> and StaticDuration. These new types provide API-compatible alternatives with stable memory layouts suitable for shared memory communication.
Changes:
- Introduced
StaticOption<T>with comprehensive API matching standardOption<T> - Introduced
StaticDurationas a zero-copy safe alternative toDuration - Removed
Option<T>,Result<T, E>, andDurationfromZeroCopySenddefault implementations - Migrated all internal usage from
OptiontoStaticOptionin service static configurations - Updated event service configuration to use new zero-copy safe types
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
iceoryx2-bb/container/src/static_option.rs |
New StaticOption<T> implementation with full API compatibility |
iceoryx2-bb/container/tests/static_option_tests.rs |
Comprehensive test suite for StaticOption |
iceoryx2-bb/container/src/lib.rs |
Module export for static_option |
iceoryx2-bb/posix/src/clock.rs |
New StaticDuration type with conversions |
iceoryx2-bb/elementary-traits/src/zero_copy_send.rs |
Removed Option, Result, and Duration from trait implementations |
iceoryx2/src/service/static_config/event.rs |
Migrated to StaticOption and StaticDuration in event configuration |
iceoryx2/src/service/builder/event.rs |
Updated event builder to use new types |
iceoryx2/src/service/mod.rs |
Updated service code to work with StaticOption |
iceoryx2/src/port/notifier.rs |
Updated to handle StaticOption and StaticDuration conversions |
iceoryx2/src/port/listener.rs |
Updated listener deadline handling |
iceoryx2-ffi/python/src/port_factory_event.rs |
Changed from copy to clone for static config access |
doc/release-notes/iceoryx2-unreleased.md |
Documented new features and API breaking changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a8e1c64 to
d721733
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1313 +/- ##
==========================================
+ Coverage 77.86% 77.90% +0.03%
==========================================
Files 405 406 +1
Lines 38759 38941 +182
Branches 1256 1265 +9
==========================================
+ Hits 30179 30336 +157
- Misses 7547 7801 +254
+ Partials 1033 804 -229
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
9c36497 to
56b689f
Compare
elBoberido
left a comment
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.
Still need to review the tests, but in the meantime, you can already have a look at the comments.
| fn when_empty_as_option_returns_empty_option() { | ||
| let sut = RelocatableOption::<i32>::default(); | ||
|
|
||
| assert_that!(sut.as_option_ref(), eq None); |
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.
If you really want to check if you get an Option with a reference to the value, the current test is not sufficient. You would need to do a pattern matching in a similar way to this
| assert_that!(sut.as_option_ref(), eq None); | |
| assert_that!(sut.as_option_ref(), eq Option::<&i32>::None); |
Same with the other tests.
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.
This is less of a unit test but more of an API tests, so that the function name and the return type are correct. It would be sufficient to test this once for every function since the compiler and the language ensures that this does not change during runtime thanks to no function overload, the rust compiler etc.
So I add this once, for every as_option* function.
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.
The idea was also to get rid of the unwraps. We have too many of them already 😅
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.
Why do you think that unwrap() is bad in a test?
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.
In this specific case it's not that bad, because the error message will be that an empty Option is accessed. Not using unwrap would only lead to a slightly better error message. It would be just to train the muscle memory to not use unwrap.
The bigger issue is when the SUT ist not an Option but something else. For example, we have some flaky tests, which I wanted to debug but since there is an unwrap on a Result, the underlying error is hidden and it makes it hard to understand what is going on
Notes for Reviewer
Option)ZeroCopySendcapableStaticOptionOptionandDurationfromZeroCopySendtrait default implStaticDurationthat implementsZeroCopySend.Pre-Review Checklist for the PR Author
Convert to draft)iox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)PR Reviewer Reminders
References
Closes #1312