new internal event functions with ownership handling (ownership used for linux implementation only) #1222
Conversation
gOpenNamedEvent can handle ownership
rex-schilasky
changed the title
There was a problem hiding this comment.
See my comments.
Also I wonder if we should deprecate the whole ecal event API and make it private for eCAL 6.
There is no reason for it being public. One can think of it as a different library / API, but it has no business in the higher level API.
Maybe this was due to testing purposes? But there are other ways to achieve the same.
Also, I don't quite understand why they need to be global functions in the first place. Why not objects, like you then have internally with CNamedEvent....
ecal/core/include/ecal/ecal_event.h
Outdated
|
|
||
| /** | ||
| * @brief Open a named event with ownership. | ||
| * |
There was a problem hiding this comment.
The comment is wrong. However, do unnamed Events have ownership? How do unnamed events communicate?
There was a problem hiding this comment.
This is fixed ..
| @@ -302,9 +321,10 @@ namespace eCAL | |||
| class CNamedEvent | |||
| { | |||
| public: | |||
| explicit CNamedEvent(const std::string& name_) : | |||
| explicit CNamedEvent(const std::string& name_, bool ownership_) : | |||
There was a problem hiding this comment.
You don't have to change it, but I would like to make a remark. I strongly dislike the bool for ownership vs non-ownership.
This should be two different classed, e.g. OwnedNamedEvent and NonOwnedNamedEvent/WeakNamedEvent.
This has two advantages:
a) To everyone reading the code that is using the events, it's clear from the type, if the event is the owner / creator or not
b) You can use inheritance. You don't need to do stuff like if(m_owner) ... which makes code clearer and more readable.
There was a problem hiding this comment.
Also, shouldn't the owner always create the event? In the code this is not tied together?
There was a problem hiding this comment.
I just wanted to "just fix the issue" and introduce as little change as possible. Let's redesign this in a separate issue.
The same for the creation. The behavior is now exact the same as before. The event is created by the first call of OpenEvent independently from the new ownership attribute. The ownership only plays a role in the destruction phase.
| @@ -923,8 +919,6 @@ TEST(IO, ZeroPayloadMessageTCP) | |||
| } | |||
| #endif | |||
|
|
|||
| #include <ecal/msg/string/publisher.h> | |||
| #include <ecal/msg/string/subscriber.h> | |||
There was a problem hiding this comment.
These includes should be needed but maybe we can move them to the top?
There was a problem hiding this comment.
These includes should be needed but maybe we can move them to the top?
They are there and have been there before ..
…ternal API (ecal_event_internal.h) to be fully API/ABI compatible again
rex-schilasky
changed the title
rex-schilasky
changed the title
rex-schilasky
changed the title
|
Tested with the following setup https://github.com/rex-schilasky/ecal_dynamic_pubsub_test |
… used for linux implementation only) (#1222)
… used for linux implementation only) (#1222)
Description
Introducing ownership flag for named event creation.
Related issues
Fixes #1218
Cherry-pick to