[WIP] [#1911] Add bag concept - #1926
Conversation
aaf7d2f to
c568d17
Compare
elfenpiff
left a comment
elfenpiff
left a comment
There was a problem hiding this comment.
Looks good - except the hardest part, the naming ;) .
| unsafe { self.remove(handle, mode) } | ||
| } | ||
|
|
||
| unsafe fn get_state(&self) -> ContainerState<T> { |
There was a problem hiding this comment.
Should we rename the ContainerState to BagState?
There was a problem hiding this comment.
I was also thinking about re-exporting all the types from iceoryx2-bb-lock-free in the iceoryx2-cal/bag namespace. Not sure about changing also the iceoryx2-bb-lock-free Container itself right now.
|
|
||
| unsafe fn remove( | ||
| &self, | ||
| handle: ContainerHandle, |
There was a problem hiding this comment.
Should we rename the ContainerHandle to BagHandle
| impl<T: Copy + Debug + ZeroCopySend> BagValue for T {} | ||
|
|
||
| /// TODO documentation | ||
| pub trait Bag: Debug + 'static { |
There was a problem hiding this comment.
Could we rename this: Bag -> BagFamily and the BagAccess is Bag.
The thinking is, that a Bag is a thing that contains T and has the following interface and behavior. But with this is a Bag a trait that defines a type alias and BagAccess is the thing that contains T.
We already have this name scheme in place in iceoryx2. We have for instance a Pointer<T> trait: https://github.com/eclipse-iceoryx/iceoryx2/blob/main/iceoryx2-bb/elementary-traits/src/pointer.rs
and a PointerFamily trait: https://github.com/eclipse-iceoryx/iceoryx2/blob/main/iceoryx2-bb/elementary-traits/src/pointer_family.rs
This reads then in code naturally:
struct SomeThing<P: Pointer> {
fuu: P<u64>
}or when you need it more generic:
struct SomeThing<P: PointerFamily> {
fuu: P::Pointer<u64>,
bar: P::Pointer<i32>
}I think I had some discussion with ?Jeff? and a LLM until come to this naming style.
| &self, | ||
| value: T, | ||
| owner_id: OwnerId, | ||
| ) -> Result<(*const T, ContainerHandle), ContainerAddFailure> { |
There was a problem hiding this comment.
Could we rename all errors to BagAddFailure etc..
|
|
||
| /// TODO documentation | ||
| pub trait Bag: Debug + 'static { | ||
| type Container<T: BagValue>: Debug |
There was a problem hiding this comment.
This we could rename to Bag and technically T would be the BagType.
There was a problem hiding this comment.
Just to prevent misunderstanding. This is your suggestion, right
pub trait BagFamily: Debug + 'static {
type Bag<T: BagType>: Debug
f1357c6 to
04a8510
Compare
| Ok(ReleaseState::Unlocked) => Ok(DeregisterNodeState::HasOwners), | ||
| Err(ContainerRemoveError::ContainerHandleNotOwnedByContainer) => { | ||
| fail!(from self, with ContainerRemoveError::ContainerHandleNotOwnedByContainer, | ||
| Err(BagRemoveError::ContainerHandleNotOwnedByContainer) => { |
There was a problem hiding this comment.
@elfenpiff this is not nice. We either have to create a BagRemoveError enum and map the errors 1-1 to ContainerRemoveError or rename the error e.g. to HandleNotOwnedByContainer which is a bit less confusing. The former option would add some boilerplate. Alternatively, the Container itself could be renamed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1926 +/- ##
==========================================
+ Coverage 76.33% 76.35% +0.02%
==========================================
Files 457 458 +1
Lines 46068 46112 +44
Branches 1489 1489
==========================================
+ Hits 35165 35210 +45
+ Misses 9641 9640 -1
Partials 1262 1262
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
04a8510 to
b08d3f4
Compare
b08d3f4 to
c79a037
Compare
2 participants
Notes for Reviewer
Pre-Review Checklist for the PR Author
Convert to draft)iox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)PR Reviewer Reminders
References
Closes #1911