-
Notifications
You must be signed in to change notification settings - Fork 498
Implemented TODO to make asynchronous destruction of presentation entities #397
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
Conversation
…cation #docs #sonar
# Conflicts: # docs/examples/1_presentation/example_10_posix_udp.cpp # include/libcyphal/presentation/presentation.hpp
Set default PMR, so that default behavior of `std::pmr::polymorphic_allocator` stays with specific memory resource (instead of default "new/delete" one) #verification #docs #sonar
# Conflicts: # .github/workflows/sonar.yml # .github/workflows/tests.yml
# Conflicts: # .github/workflows/sonar.yml # .github/workflows/tests.yml # docs/examples/1_presentation/example_1_presentation_1_ping_user_service_udp.cpp # docs/examples/2_application/example_2_application_0_node_hb_getinfo_udp.cpp # include/libcyphal/application/registry/register.hpp # include/libcyphal/presentation/client.hpp # include/libcyphal/presentation/publisher.hpp # include/libcyphal/presentation/server.hpp # test/unittest/application/node/test_get_info_provider.cpp
…ities (publishers, subscribers and rpc clients). Now unreferenced shared objects (with ref_count==0) will be quickly appended to the double-linked list, which will be traversed asynchronously in a near future callback.
| CETL_DEBUG_ASSERT(shared_node.isLinked(), ""); | ||
| CETL_DEBUG_ASSERT(!shared_node.isReferenced(), ""); | ||
|
|
||
| // TODO: make it async (deferred to "on idle" callback). |
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 the "TODO" this PR is about.
|
thirtytwobits
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.
No blockers
| // No Lint b/c this `UnRefNode` is a simple helper struct as base of the below `SharedObject`. | ||
| // It's under `detail` namespace and not supposed to be used directly by the users of the library. | ||
| // NOLINTBEGIN(misc-non-private-member-variables-in-classes) | ||
| UnRefNode* prev_node{nullptr}; |
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.
could we protect these by making them private then using friend class SharedObject?
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.
we probably can but I'm not sure we need - all derived classes (from SharedObject) are still "Impl" internal (and under detail namespace) entities. F.e. if you work with Publisher<Message> (which is not shared object!) then you don't see under the hood involved PublisherImpl. The same story for 2 other entities: subscriber and rpc client.
| Presentation& operator=(const Presentation&) = delete; | ||
| Presentation& operator=(Presentation&&) noexcept = delete; | ||
|
|
||
| ~Presentation() |
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.
should this be virtual? I'm not sure TBH.
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.
Presentation is final, so I don't think we need virtual.



Implemented TODO to make asynchronous destruction of presentation entities (publishers, subscribers and rpc clients).
Now unreferenced shared objects (with ref_count==0) will be quickly appended to the double-linked list, which will be traversed asynchronously in a near future callback (schedule for
now()).