[core] redesign subscriber api #1863
Conversation
CPublisherImpl event handling renamings
RemXY -> RemoveXY Logging corrected
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| bool CSubscriber::Create(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const Subscriber::Configuration& config_) | ||
| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_) : |
There was a problem hiding this comment.
warning: the const qualified parameter 'event_callback_' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
ecal/core/include/ecal/ecal_subscriber.h:69:
- CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_ = GetSubscriberConfiguration());
+ CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT& event_callback_, const Subscriber::Configuration& config_ = GetSubscriberConfiguration());| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_) : | |
| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT& event_callback_, const Subscriber::Configuration& config_) : |
There was a problem hiding this comment.
Rex: did you want to turn off this warning? doesn't it make sense in this case? Why not a reference?
| } | ||
|
|
||
| bool CSubscriber::Create(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const Subscriber::Configuration& config_) | ||
| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_) : |
There was a problem hiding this comment.
warning: the const qualified parameter 'event_callback_' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
ecal/core/include/ecal/ecal_subscriber.h:67:
- CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_ = GetSubscriberConfiguration());
+ CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT& event_callback_, const Subscriber::Configuration& config_ = GetSubscriberConfiguration());| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT event_callback_, const Subscriber::Configuration& config_) : | |
| CSubscriber::CSubscriber(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const SubEventIDCallbackT& event_callback_, const Subscriber::Configuration& config_) : |
|
|
||
| int content(static_cast<int>(static_cast<unsigned char*>(data_->buf)[0])); | ||
| int content(static_cast<int>(static_cast<unsigned char*>(data_.buf)[0])); |
There was a problem hiding this comment.
warning: variable 'content' of type 'int' can be declared 'const' [misc-const-correctness]
| int content(static_cast<int>(static_cast<unsigned char*>(data_.buf)[0])); | |
| int const content(static_cast<int>(static_cast<unsigned char*>(data_.buf)[0])); |
| std::cout << " Id : " << data_->id << std::endl; | ||
| std::cout << " Time : " << data_->time << std::endl; | ||
| std::cout << " Clock : " << data_->clock << std::endl; | ||
| std::cout << " Size : " << data_.size << std::endl; |
There was a problem hiding this comment.
warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]
| std::cout << " Size : " << data_.size << std::endl; | |
| std::cout << " Size : " << data_.size << '\n'; |
| std::cout << " Time : " << data_->time << std::endl; | ||
| std::cout << " Clock : " << data_->clock << std::endl; | ||
| std::cout << " Size : " << data_.size << std::endl; | ||
| std::cout << " Id : " << data_.id << std::endl; |
There was a problem hiding this comment.
warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]
| std::cout << " Id : " << data_.id << std::endl; | |
| std::cout << " Id : " << data_.id << '\n'; |
| std::cout << " Clock : " << data_->clock << std::endl; | ||
| std::cout << " Size : " << data_.size << std::endl; | ||
| std::cout << " Id : " << data_.id << std::endl; | ||
| std::cout << " Time : " << data_.time << std::endl; |
There was a problem hiding this comment.
warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]
| std::cout << " Time : " << data_.time << std::endl; | |
| std::cout << " Time : " << data_.time << '\n'; |
| std::cout << " Size : " << data_.size << std::endl; | ||
| std::cout << " Id : " << data_.id << std::endl; | ||
| std::cout << " Time : " << data_.time << std::endl; | ||
| std::cout << " Clock : " << data_.clock << std::endl; |
There was a problem hiding this comment.
warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]
| std::cout << " Clock : " << data_.clock << std::endl; | |
| std::cout << " Clock : " << data_.clock << '\n'; |
| eCAL::Initialize(config, "callback_topic_id"); | ||
| } | ||
|
|
||
| void TearDown() override |
There was a problem hiding this comment.
warning: method 'TearDown' can be made static [readability-convert-member-functions-to-static]
| void TearDown() override | |
| static void TearDown() override |
| } | ||
| }; | ||
|
|
||
| TEST_P(TestFixture, OnePubSub) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST_P(TestFixture, OnePubSub) | |
| TEST_P(TestFixture /*unused*/, OnePubSub /*unused*/) |
| } | ||
| }; | ||
|
|
||
| TEST_P(TestFixture, OnePubSub) |
There was a problem hiding this comment.
warning: the parameter #1 is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
| TEST_P(TestFixture, OnePubSub) | |
| TEST_P(const TestFixture&, OnePubSub) |
| EXPECT_EQ(callback_datatype_info, datatype_info); | ||
| } | ||
|
|
||
| TEST_P(TestFixture, MultiplePubSub) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST_P(TestFixture, MultiplePubSub) | |
| TEST_P(TestFixture /*unused*/, MultiplePubSub /*unused*/) |
| EXPECT_EQ(callback_datatype_info, datatype_info); | ||
| } | ||
|
|
||
| TEST_P(TestFixture, MultiplePubSub) |
There was a problem hiding this comment.
warning: the parameter #1 is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
| TEST_P(TestFixture, MultiplePubSub) | |
| TEST_P(const TestFixture&, MultiplePubSub) |
| @@ -68,25 +68,25 @@ namespace eCAL | |||
|
|
|||
|
|
|||
| template <typename T, typename DynamicDeserializer> | |||
| class CDynamicMessageSubscriber final : public CSubscriber | |||
| class CDynamicMessageSubscriber final : public v5::CSubscriber | |||
There was a problem hiding this comment.
warning: constructor does not initialize these fields: m_cb_callback, m_error_callback [cppcoreguidelines-pro-type-member-init]
ecal/core/include/ecal/msg/dynamic.h:258:
- MsgReceiveCallbackT m_cb_callback;
+ MsgReceiveCallbackT m_cb_callback{};ecal/core/include/ecal/msg/dynamic.h:260:
- ErrorCallbackT m_error_callback;
+ ErrorCallbackT m_error_callback{};| #include <string> | ||
| #include <thread> | ||
|
|
||
| #include <gtest/gtest.h> |
There was a problem hiding this comment.
warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]
#include <gtest/gtest.h>
^|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| TEST(core_cpp_pubsub_v5, TestSubscriberIsPublishedTiming) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST(core_cpp_pubsub_v5, TestSubscriberIsPublishedTiming) | |
| TEST(core_cpp_pubsub_v5 /*unused*/, TestSubscriberIsPublishedTiming /*unused*/) |
| eCAL::Finalize(); | ||
| } | ||
|
|
||
| TEST(core_cpp_pubsub_v5, TestPublisherIsSubscribedTiming) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST(core_cpp_pubsub_v5, TestPublisherIsSubscribedTiming) | |
| TEST(core_cpp_pubsub_v5 /*unused*/, TestPublisherIsSubscribedTiming /*unused*/) |
| eCAL::Finalize(); | ||
| } | ||
|
|
||
| TEST(core_cpp_pubsub_v5, TestChainedPublisherSubscriberCallback) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST(core_cpp_pubsub_v5, TestChainedPublisherSubscriberCallback) | |
| TEST(core_cpp_pubsub_v5 /*unused*/, TestChainedPublisherSubscriberCallback /*unused*/) |
| @@ -384,7 +385,7 @@ | |||
| topic_info.encoding = topic_enc_; | |||
| topic_info.descriptor = std::string(topic_desc_, static_cast<size_t>(topic_desc_length_)); | |||
|
|
|||
| auto* sub = new eCAL::CSubscriber; | |||
| auto* sub = new eCAL::v5::CSubscriber; | |||
| if (!sub->Create(topic_name_, topic_info)) | |||
| { | |||
| delete sub; | |||
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete sub;
^Additional context
lang/python/core/src/ecal_clang.cpp:387: variable declared here
auto* sub = new eCAL::v5::CSubscriber;
^| @@ -398,7 +399,7 @@ | |||
| /****************************************/ | |||
| bool sub_destroy(ECAL_HANDLE handle_) | |||
| { | |||
| auto* sub = static_cast<eCAL::CSubscriber*>(handle_); | |||
| auto* sub = static_cast<eCAL::v5::CSubscriber*>(handle_); | |||
| if(sub != nullptr) | |||
| { | |||
| delete sub; | |||
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete sub;
^Additional context
lang/python/core/src/ecal_clang.cpp:401: variable declared here
auto* sub = static_cast<eCAL::v5::CSubscriber*>(handle_);
^| @@ -422,7 +423,7 @@ PyObject* sub_set_callback(PyObject* /*self*/, PyObject* args) | |||
| if (!PyArg_ParseTuple(args, "nO", &topic_handle, &cb_func)) | |||
| return nullptr; | |||
|
|
|||
| eCAL::CSubscriber* sub = (eCAL::CSubscriber*)topic_handle; | |||
| eCAL::v5::CSubscriber* sub = (eCAL::v5::CSubscriber*)topic_handle; | |||
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
eCAL::v5::CSubscriber* sub = (eCAL::v5::CSubscriber*)topic_handle;
^| @@ -475,7 +476,7 @@ | |||
| if (!PyArg_ParseTuple(args, "nO", &topic_handle, &cb_func)) | |||
| return nullptr; | |||
|
|
|||
| eCAL::CSubscriber* sub = (eCAL::CSubscriber*)topic_handle; | |||
| eCAL::v5::CSubscriber* sub = (eCAL::v5::CSubscriber*)topic_handle; | |||
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
eCAL::v5::CSubscriber* sub = (eCAL::v5::CSubscriber*)topic_handle;
^| { | ||
| std::string topicName = topic_name_; | ||
| std::string topicName = topic_id_.topic_name; |
There was a problem hiding this comment.
warning: variable 'topicName' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]
| std::string topicName = topic_id_.topic_name; | |
| std::string const topicName = topic_id_.topic_name; |
Description
Changes Summary:
Namespace Updates:
eCAL::CSubscriberAPI has been moved to theeCAL::v5namespace for compatibility with existing systems using eCAL version 5.eCAL::CSubscriberAPI under theeCAL::v6namespace (default inline namespace).API Modernization:
CreateandDestroymethods inv6. Subscribers are now fully initialized via constructors and cleaned up automatically via destructors.AddEventCallback,RemEventCallback) are now passed as optional parameters during object construction, simplifying event handling.AddReceiveCallbacktoSetReceiveCallback,RemReceiveCallbacktoRemoveReceiveCallbackSetReceiveCallbackandRemoveReceiveCallbackmethods remain part of the API for flexibility in managing message reception callbacks.SetAttribute,ClearAttribute, andSetIDwere removed as they were experimental or rarely used.ReceiveBuffermethod inv5was replaced withSetReceiveCallbackto encourage a more event-driven approach to data reception inv6.Code Refactoring:
std::shared_ptr<CSubscriberImpl>for robust resource management and memory safety.SDataTypeInformationfor specifying topic metadata and aSubEventIDCallbackTfor event-driven designs.Improved Usability:
Impact:
eCAL::v5::CSubscriberwhile offering a modern, streamlined API for new applications.