-
Notifications
You must be signed in to change notification settings - Fork 174
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
Template class for CMessageSubscriber (instead of CMsgSubscriber) #1582
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.
clang-tidy made some suggestions
{ | ||
class CDynamicSubscriber | ||
class CapnprotoDynamicDeserializer |
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.
warning: constructor does not initialize these fields: m_msg_builder_, m_schema_map, m_loader [cppcoreguidelines-pro-type-member-init]
ecal/core/include/ecal/msg/capnproto/dynamic.h:81:
- capnp::MallocMessageBuilder m_msg_builder_;
- std::map<SDataTypeInformation, capnp::Schema> m_schema_map;
- capnp::SchemaLoader m_loader;
+ capnp::MallocMessageBuilder m_msg_builder_{};
+ std::map<SDataTypeInformation, capnp::Schema> m_schema_map{};
+ capnp::SchemaLoader m_loader{};
{ | ||
// we leave topic info empty | ||
SDataTypeInformation topic_info; |
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.
warning: variable 'topic_info' is not initialized [cppcoreguidelines-init-variables]
SDataTypeInformation topic_info; | |
SDataTypeInformation topic_info = 0; |
// This function is NOT threadsafe!!! | ||
// what about the lifetime of the objects? | ||
// It's totally unclear to me :/ | ||
bool Deserialize(typename capnp::DynamicStruct::Reader& msg_, const void* buffer_, size_t size_, const SDataTypeInformation& datatype_info_) |
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.
warning: method 'Deserialize' can be made static [readability-convert-member-functions-to-static]
bool Deserialize(typename capnp::DynamicStruct::Reader& msg_, const void* buffer_, size_t size_, const SDataTypeInformation& datatype_info_) | |
static bool Deserialize(typename capnp::DynamicStruct::Reader& msg_, const void* buffer_, size_t size_, const SDataTypeInformation& datatype_info_) |
msg_callback = callback_; | ||
return subscriber.AddReceiveCallback(std::bind(&CDynamicSubscriber::OnReceive, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5)); | ||
} | ||
capnp::Schema schema = GetSchema(datatype_info_); |
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.
warning: variable 'schema' is not initialized [cppcoreguidelines-init-variables]
capnp::Schema schema = GetSchema(datatype_info_); | |
capnp::Schema schema = 0 = GetSchema(datatype_info_); |
**/ | ||
bool Receive(T& msg_, long long* time_ = nullptr, int rcv_timeout_ = 0) | ||
{ | ||
std::string rec_buf; |
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.
warning: variable 'rec_buf' is not initialized [cppcoreguidelines-init-variables]
std::string rec_buf; | |
std::string rec_buf = 0; |
bool Receive(T& msg_, long long* time_ = nullptr, int rcv_timeout_ = 0) | ||
{ | ||
std::string rec_buf; | ||
bool success = CSubscriber::ReceiveBuffer(rec_buf, time_, rcv_timeout_); |
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.
warning: variable 'success' is not initialized [cppcoreguidelines-init-variables]
bool success = CSubscriber::ReceiveBuffer(rec_buf, time_, rcv_timeout_); | |
bool success = false = CSubscriber::ReceiveBuffer(rec_buf, time_, rcv_timeout_); |
* @param clock_ Message writer clock. | ||
* @param id_ Message id. | ||
**/ | ||
typedef std::function<void(const char* topic_name_, const T& msg_, long long time_, long long clock_, long long id_)> MsgReceiveCallbackT; |
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.
warning: use 'using' instead of 'typedef' [modernize-use-using]
typedef std::function<void(const char* topic_name_, const T& msg_, long long time_, long long clock_, long long id_)> MsgReceiveCallbackT; | |
using MsgReceiveCallbackT = std::function<void (const char *, const T &, long long, long long, long long)>; |
**/ | ||
bool RemReceiveCallback() | ||
{ | ||
bool ret = CSubscriber::RemReceiveCallback(); |
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.
warning: variable 'ret' is not initialized [cppcoreguidelines-init-variables]
bool ret = CSubscriber::RemReceiveCallback(); | |
bool ret = false = CSubscriber::RemReceiveCallback(); |
@@ -88,9 +88,9 @@ int main(int argc, char **argv) | |||
while (eCAL::Ok()) | |||
{ | |||
// receive content | |||
if (sub.Receive(nullptr, 0)) | |||
AddressBook::Reader reader; |
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.
warning: variable 'reader' is not initialized [cppcoreguidelines-init-variables]
AddressBook::Reader reader; | |
AddressBook::Reader reader = 0; |
53d67cb
to
c050e9e
Compare
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 20 out of 35. Check the log or trigger a new build to see more.
@@ -261,7 +261,7 @@ class TreeMessageVisitor : public MessageVisitor | |||
|
|||
} | |||
|
|||
void PopulateProtoTree(ftxui::TreeNode &root, google::protobuf::Message *message, const std::shared_ptr<StyleSheet> style) | |||
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) |
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.
warning: function 'PopulateProtoTree' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style)
^
Additional context
app/mon/mon_tui/src/tui/view/message_visualization/proto_tree.hpp:263: make as 'inline'
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style)
^
@@ -261,7 +261,7 @@ | |||
|
|||
} | |||
|
|||
void PopulateProtoTree(ftxui::TreeNode &root, google::protobuf::Message *message, const std::shared_ptr<StyleSheet> style) | |||
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) |
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.
warning: the const qualified parameter 'style' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) | |
void PopulateProtoTree(ftxui::TreeNode &root, const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet>& style) |
@@ -272,7 +272,7 @@ | |||
} | |||
} | |||
|
|||
ftxui::TreeNodePtr ProtoTree(google::protobuf::Message *message, const std::shared_ptr<StyleSheet> style) | |||
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) |
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.
warning: function 'ProtoTree' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style)
^
Additional context
app/mon/mon_tui/src/tui/view/message_visualization/proto_tree.hpp:274: make as 'inline'
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style)
^
@@ -272,7 +272,7 @@ | |||
} | |||
} | |||
|
|||
ftxui::TreeNodePtr ProtoTree(google::protobuf::Message *message, const std::shared_ptr<StyleSheet> style) | |||
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) |
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.
warning: the const qualified parameter 'style' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet> style) | |
ftxui::TreeNodePtr ProtoTree(const std::shared_ptr<google::protobuf::Message>& message, const std::shared_ptr<StyleSheet>& style) |
{ | ||
class CDynamicSubscriber | ||
class CapnprotoDynamicDeserializer |
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.
warning: constructor does not initialize these fields: m_msg_builder_, m_schema_map, m_loader [cppcoreguidelines-pro-type-member-init]
ecal/core/include/ecal/msg/capnproto/dynamic.h:77:
- capnp::MallocMessageBuilder m_msg_builder_;
- std::map<SDataTypeInformation, capnp::Schema> m_schema_map;
- capnp::SchemaLoader m_loader;
+ capnp::MallocMessageBuilder m_msg_builder_{};
+ std::map<SDataTypeInformation, capnp::Schema> m_schema_map{};
+ capnp::SchemaLoader m_loader{};
, m_topic_name(topic_name_) | ||
, m_deserializer() | ||
{ | ||
CSubscriber::Create(topic_name_); |
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.
warning: variable 'topic_name_' is not initialized [cppcoreguidelines-init-variables]
CSubscriber::Create(topic_name_); | |
CSubscriber::Create(topic_name_ = 0); |
/** | ||
* @brief Move Constructor | ||
**/ | ||
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs) |
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.
warning: constructor does not initialize these fields: m_error_callback [cppcoreguidelines-pro-type-member-init]
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs)
^
/** | ||
* @brief Move Constructor | ||
**/ | ||
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs) |
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.
warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
ecal/core/include/ecal/msg/dynamic.h:112:
- : CSubscriber(std::move(rhs))
+ noexcept : CSubscriber(std::move(rhs))
// the callback bound to the CSubscriber belongs to rhs, bind to this callback instead | ||
CSubscriber::RemReceiveCallback(); | ||
auto callback = std::bind(&CDynamicMessageSubscriber::ReceiveCallback, this, std::placeholders::_1, std::placeholders::_2); | ||
CSubscriber::AddReceiveCallback(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.
warning: variable 'callback' is not initialized [cppcoreguidelines-init-variables]
CSubscriber::AddReceiveCallback(callback); | |
CSubscriber::AddReceiveCallback(callback = 0); |
// Do we want to call error callbacks on receive? Probably not! std::expected wouuld be a good thing to return the reason why things went wrong. | ||
std::optional<T> Receive(long long* time_ = nullptr, int rcv_timeout_ = 0) | ||
{ | ||
std::string rec_buf; |
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.
warning: variable 'rec_buf' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]
std::string rec_buf; | |
std::string const rec_buf; |
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.
clang-tidy made some suggestions
@@ -58,7 +58,7 @@ class ProtoDynSubscriberTest : public ::testing::Test { | |||
pub.Send(p); | |||
} | |||
|
|||
void OnPerson(const char*, const google::protobuf::Message&, long long) | |||
void OnPerson(const char*, const std::shared_ptr<google::protobuf::Message>&, long long) |
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.
warning: all parameters should be named in a function [readability-named-parameter]
void OnPerson(const char*, const std::shared_ptr<google::protobuf::Message>&, long long) | |
void OnPerson(const char* /*unused*/, const std::shared_ptr<google::protobuf::Message>& /*unused*/, long long /*unused*/) |
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.
clang-tidy made some suggestions
/** | ||
* @brief Move Constructor | ||
**/ | ||
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs) |
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.
warning: constructor does not initialize these fields: m_cb_callback, m_error_callback [cppcoreguidelines-pro-type-member-init]
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs)
^
, m_cb_callback(std::move(rhs.m_cb_callback)) | ||
, m_deserializer(std::move(rhs.m_deserializer)) | ||
{ | ||
bool has_callback = (m_cb_callback != 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.
warning: variable 'has_callback' is not initialized [cppcoreguidelines-init-variables]
bool has_callback = (m_cb_callback != nullptr); | |
bool has_callback = false = (m_cb_callback != nullptr); |
private: | ||
void ReceiveCallback(const char* topic_name_, const struct eCAL::SReceiveCallbackData* data_) | ||
{ | ||
MsgReceiveCallbackT fn_callback = 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.
warning: variable 'fn_callback' is not initialized [cppcoreguidelines-init-variables]
MsgReceiveCallbackT fn_callback = nullptr; | |
MsgReceiveCallbackT fn_callback = 0 = nullptr; |
|
||
void CallErrorCallback(const std::string& message) | ||
{ | ||
ErrorCallbackT error_callback = 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.
warning: variable 'error_callback' is not initialized [cppcoreguidelines-init-variables]
ErrorCallbackT error_callback = nullptr; | |
ErrorCallbackT error_callback = 0 = nullptr; |
…o inheritance required. For CProtoDynSubscriber implies API change.
b16e400
to
9154c9f
Compare
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.
👍
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.
clang-tidy made some suggestions
{ | ||
class CDynamicSubscriber | ||
class CapnprotoDynamicDeserializer |
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.
warning: constructor does not initialize these fields: m_msg_builder, m_schema_map, m_loader [cppcoreguidelines-pro-type-member-init]
ecal/core/include/ecal/msg/capnproto/dynamic.h:77:
- capnp::MallocMessageBuilder m_msg_builder;
- std::map<SDataTypeInformation, capnp::Schema> m_schema_map;
- capnp::SchemaLoader m_loader;
+ capnp::MallocMessageBuilder m_msg_builder{};
+ std::map<SDataTypeInformation, capnp::Schema> m_schema_map{};
+ capnp::SchemaLoader m_loader{};
{ | ||
if (!initialized) | ||
capnp::Schema schema = GetSchema(datatype_info_); | ||
capnp::DynamicStruct::Builder root_builder = m_msg_builder.getRoot<capnp::DynamicStruct>(schema.asStruct()); |
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.
warning: variable 'root_builder' is not initialized [cppcoreguidelines-init-variables]
capnp::DynamicStruct::Builder root_builder = m_msg_builder.getRoot<capnp::DynamicStruct>(schema.asStruct()); | |
capnp::DynamicStruct::Builder root_builder = 0 = m_msg_builder.getRoot<capnp::DynamicStruct>(schema.asStruct()); |
**/ | ||
class CBuilderSubscriber : public CMsgSubscriber<capnp::MallocMessageBuilder> | ||
template <typename T> | ||
class CapnprotoDeserializer |
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.
warning: constructor does not initialize these fields: m_msg_builder [cppcoreguidelines-pro-type-member-init]
ecal/core/include/ecal/msg/capnproto/subscriber.h:69:
- capnp::MallocMessageBuilder m_msg_builder;
+ capnp::MallocMessageBuilder m_msg_builder{};
/** | ||
* @brief Constructor. | ||
**/ | ||
CDynamicMessageSubscriber() : CSubscriber() |
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.
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:319:
- MsgReceiveCallbackT m_cb_callback;
+ MsgReceiveCallbackT m_cb_callback{};
ecal/core/include/ecal/msg/dynamic.h:321:
- ErrorCallbackT m_error_callback;
+ ErrorCallbackT m_error_callback{};
/** | ||
* @brief Move Constructor | ||
**/ | ||
CDynamicMessageSubscriber(CDynamicMessageSubscriber&& rhs) |
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.
warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
ecal/core/include/ecal/msg/dynamic.h:111:
- : CSubscriber(std::move(rhs))
+ noexcept : CSubscriber(std::move(rhs))
/** | ||
* @brief Constructor. | ||
**/ | ||
CMessageSubscriber() : CSubscriber() |
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.
warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]
ecal/core/include/ecal/msg/subscriber.h:249:
- {
- }
+ = default;
/** | ||
* @brief Move Constructor | ||
**/ | ||
CMessageSubscriber(CMessageSubscriber&& rhs) |
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.
warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
ecal/core/include/ecal/msg/subscriber.h:283:
- : CSubscriber(std::move(rhs))
+ noexcept : CSubscriber(std::move(rhs))
/** | ||
* @brief Move assignment | ||
**/ | ||
CMessageSubscriber& operator=(CMessageSubscriber&& rhs) |
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.
warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
ecal/core/include/ecal/msg/subscriber.h:302:
- {
+ noexcept {
No inheritance required Dynamic Subscribers (capnproto) not yet working. First need DatatypeInformation available in callback.
First PR to greatly simplify message subscribers.