Skip to content
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

eCAL core: provide TopicInformation (encoding, type, descriptor) instead of 2 strings (type, schema). #1126

Merged
merged 4 commits into from
Jun 16, 2023

Conversation

KerstinKeller
Copy link
Contributor

eCAL core: provide TopicInformation (encoding, type, descriptor) instead of 2 strings (type, schema).

This commit contains various API changes, however made in a semver compliant way.

  • Adds various API functions and deprecates old ones
  • Adds field to topic.proto file
  • For communication compatibility, descriptor information is send out on old and new fields in proto message

Occurrences of deprecated functions fixed in eCAL Code (Samples, Apps, Tests)
Try to continue make builds.

  • Feature

Technically, this API should introduce no API breaks, only internal ones.

…ead of 2 strings (type, schema).

This commit contains various API changes, however made in a semver compliant way.
- Adds various API functions and deprecates old ones
- Adds field to topic.proto file
- For communication compatibility, descriptor information is send out on old and new fields in proto message

Occurrences of deprecated functions fixed in eCAL Code (Samples, Apps, Tests)
Try to continue make builds.
Copy link
Contributor

@github-actions github-actions bot left a 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 25. Check the log or trigger a new build to see more.

@@ -118,7 +118,9 @@ namespace eCAL
{
if (!initialized)
{
std::string topic_desc = eCAL::Util::GetDescription(topic_name_);
STopicInformation topic_info_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: uninitialized record type: 'topic_info_' [cppcoreguidelines-pro-type-member-init]

Suggested change
STopicInformation topic_info_;
STopicInformation topic_info_{};

**/
std::string GetTypeName() const
STopicInformation GetTopicInformation() const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'GetTopicInformation' can be made static [readability-convert-member-functions-to-static]

Suggested change
STopicInformation GetTopicInformation() const
static STopicInformation GetTopicInformation()

{
return ("");
STopicInformation topic_info;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: uninitialized record type: 'topic_info' [cppcoreguidelines-pro-type-member-init]

Suggested change
STopicInformation topic_info;
STopicInformation topic_info{};

{
return eCAL::capnproto::TypeAsString<message_type>();
STopicInformation topic_info;
Copy link
Contributor

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]

Suggested change
STopicInformation topic_info;
STopicInformation topic_info = 0;

[[deprecated("Please use STopicInformation GetTopicInformation() instead. This function will be removed in eCAL6.")]]
virtual std::string GetTypeName() const
{
STopicInformation topic_info{ GetTopicInformation() };
Copy link
Contributor

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]

Suggested change
STopicInformation topic_info{ GetTopicInformation() };
STopicInformation topic_info = 0{ GetTopicInformation() };

@@ -58,7 +58,7 @@ namespace eCAL

// call the function via its class becase it's a virtual function that is called in constructor/destructor,-
// where the vtable is not created yet or it's destructed.
CPublisher(const std::string& topic_name_) : CMsgPublisher<T>(topic_name_, CPublisher::GetTypeName(), CPublisher::GetDescription())
CPublisher(const std::string& topic_name_) : CMsgPublisher<T>(topic_name_, GetTopicInformation())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'CPublisher::GetTopicInformation' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

      CPublisher(const std::string& topic_name_) : CMsgPublisher<T>(topic_name_, GetTopicInformation())
                                                                                 ^
Additional context

app/mon/mon_cli/src/ecal_mon_cli.cpp:551: Calling constructor for 'CPublisher<std::basic_string>'

  eCAL::string::CPublisher<std::string> pub(topic_name);
                                        ^

ecal/core/include/ecal/msg/string/publisher.h:60: Call to virtual method 'CPublisher::GetTopicInformation' during construction bypasses virtual dispatch

      CPublisher(const std::string& topic_name_) : CMsgPublisher<T>(topic_name_, GetTopicInformation())
                                                                                 ^

{
return("");
STopicInformation topic_info;
Copy link
Contributor

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]

Suggested change
STopicInformation topic_info;
STopicInformation topic_info = 0;

{
return("");
STopicInformation topic_info;
Copy link
Contributor

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]

Suggested change
STopicInformation topic_info;
STopicInformation topic_info = 0;

@@ -24,8 +24,8 @@

#pragma once

#include <ecal/ecal_publisher.h>
#include <ecal/ecal_subscriber.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'ecal/ecal_subscriber.h' file not found [clang-diagnostic-error]

#include <ecal/ecal_subscriber.h>
         ^

@FlorianReimold FlorianReimold added this to the eCAL 5.12 milestone Jun 15, 2023
@KerstinKeller KerstinKeller changed the title Feature/topic type encoding split 3 eCAL core: provide TopicInformation (encoding, type, descriptor) instead of 2 strings (type, schema). Jun 15, 2023
const char* tid; //!< topic id of the connected subscriber (for pub_event_update_connection only)
const char* ttype; //!< topic type information of the connected subscriber (for pub_event_update_connection only)
const char* tdesc; //!< topic descriptor information of the connected subscriber (for pub_event_update_connection only)
struct STopicInformationC tinfo; //!< topic information of the connected subscriber (for pub_event_update_connection only)
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take out until we fix C API completely (not with this PR)

{
/**
* @brief eCAL transport layer types.
**/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Topic Information -> not transport layer types

@@ -959,9 +961,6 @@ namespace eCAL
// direction
pMonTopic->set_direction(direction_);

// topic type
pMonTopic->set_ttype(topic.second.ttype);

// topic transport layers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove only with eCAL6!

if (m_tlayer.sm_udp_mc == TLayer::smode_none) m_tlayer.sm_udp_mc = Config::GetPublisherUdpMulticastMode();
if (m_tlayer.sm_shm == TLayer::smode_none) m_tlayer.sm_shm = Config::GetPublisherShmMode();
if (m_tlayer.sm_tcp == TLayer::smode_none) m_tlayer.sm_tcp = Config::GetPublisherTcpMode();
if (m_tlayer.sm_inproc == TLayer::smode_none) m_tlayer.sm_inproc = Config::GetPublisherInprocMode();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix alignment

m_datawriter->SetLayerMode(TLayer::tlayer_udp_mc, m_tlayer.sm_udp_mc);
m_datawriter->SetLayerMode(TLayer::tlayer_shm, m_tlayer.sm_shm);
m_datawriter->SetLayerMode(TLayer::tlayer_tcp, m_tlayer.sm_tcp);
m_datawriter->SetLayerMode(TLayer::tlayer_inproc, m_tlayer.sm_inproc);
// create it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

align!

Copy link
Contributor

@github-actions github-actions bot left a 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

[[deprecated("Please use STopicInformation GetTopicInformation() instead. This function will be removed in eCAL6.")]]
virtual std::string GetTypeName() const
{
STopicInformation topic_info{ GetTopicInformation() };
Copy link
Contributor

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]

Suggested change
STopicInformation topic_info{ GetTopicInformation() };
STopicInformation topic_info = 0{ GetTopicInformation() };

@@ -221,19 +220,19 @@ namespace eCAL
}
}

bool CPubGate::ApplyTopicToDescGate(const std::string& topic_name_, const std::string& topic_type_, const std::string& topic_desc_)
bool CPubGate::ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ApplyTopicToDescGate' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/pubsub/ecal_pubgate.h:62:

-     bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);
+     static bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);

@@ -420,21 +451,21 @@ namespace eCAL
m_tlayer = TLayer::STLayer();
}

bool CPublisher::ApplyTopicToDescGate(const std::string& topic_name_, const std::string& topic_type_, const std::string& topic_desc_)
bool CPublisher::ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ApplyTopicToDescGate' can be made static [readability-convert-member-functions-to-static]

ecal/core/include/ecal/ecal_publisher.h:537:

-     bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);
+     static bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);

@@ -365,20 +364,20 @@ namespace eCAL
return(0);
}

bool CSubGate::ApplyTopicToDescGate(const std::string& topic_name_, const std::string& topic_type_, const std::string& topic_desc_)
bool CSubGate::ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ApplyTopicToDescGate' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/pubsub/ecal_subgate.h:62:

-     bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);
+     static bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);

@@ -269,20 +292,20 @@ namespace eCAL
m_qos.reliability = QOS::best_effort_reliability_qos;
}

bool CSubscriber::ApplyTopicToDescGate(const std::string& topic_name_, const std::string& topic_type_, const std::string& topic_desc_)
bool CSubscriber::ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ApplyTopicToDescGate' can be made static [readability-convert-member-functions-to-static]

ecal/core/include/ecal/ecal_subscriber.h:348:

-     bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);
+     static bool ApplyTopicToDescGate(const std::string& topic_name_, const STopicInformation& topic_info_);

@KerstinKeller KerstinKeller merged commit 7011931 into master Jun 16, 2023
13 checks passed
@KerstinKeller KerstinKeller deleted the feature/topic-type-encoding-split-3 branch August 17, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants