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

[measurement] Reuse Serializers for High Level Measurement API #2087

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

KerstinKeller
Copy link
Contributor

Before, Serialization / Deserialization for pubsub and measurement were separated and included code duplication.
The measurement implementation now reuses the pubsub serialization implementation to provide a highl level interface to read measurements for all supported serialization formats.

@hannemn hannemn added the cherry-pick-to-NONE Don't cherry-pick these changes label Mar 21, 2025
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 91. Check the log or trigger a new build to see more.

#include <ecal/measurement/measurement.h>

namespace eCAL
{
namespace measurement
{
class IBinaryChannel
class IChannel
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: destructor of 'IChannel' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

    class IChannel
          ^
Additional context

contrib/measurement/base/include/ecal/measurement/imeasurement.h:35: make it public and virtual

    class IChannel
          ^

: channel(channel_)
, meas(meas_)
IChannel(std::shared_ptr<experimental::measurement::base::Reader> meas_, experimental::measurement::base::Channel channel_)
: meas(meas_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'meas_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

Suggested change
: meas(meas_)
: meas(std::move(meas_))

, meas(meas_)
IChannel(std::shared_ptr<experimental::measurement::base::Reader> meas_, experimental::measurement::base::Channel channel_)
: meas(meas_)
, channel(channel_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'channel_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

Suggested change
, channel(channel_)
, channel(std::move(channel_))

{
namespace measurement
{
class IBinaryChannel
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: destructor of 'IBinaryChannel' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

    class IBinaryChannel
          ^
Additional context

contrib/measurement/base/src/imeasurement.cpp:35: make it public and virtual

    class IBinaryChannel
          ^

{
public:
IBinaryChannel(std::shared_ptr<experimental::measurement::base::Reader> meas_, experimental::measurement::base::Channel channel_)
: meas(meas_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'meas_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

Suggested change
: meas(meas_)
: meas(std::move(meas_))

{
public:
OChannel(std::shared_ptr<experimental::measurement::base::Writer> meas_, std::string name_, const eCAL::experimental::measurement::base::DataTypeInformation& datatype_info)
: binary_channel(meas_, name_, datatype_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: parameter 'meas_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

Suggested change
: binary_channel(meas_, name_, datatype_info)
: binary_channel(std::move(meas_), name_, datatype_info)


OChannel<T>& operator<<(const Frame<T>& entry_)
{
eCAL::message::Serialize(entry_.message, buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no member named 'message' in namespace 'eCAL' [clang-diagnostic-error]

        eCAL::message::Serialize(entry_.message, buffer);
              ^



// Streaming operator to change the sender ID
OChannel<T>& operator<<(const SenderID& id_)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unknown type name 'SenderID' [clang-diagnostic-error]

      OChannel<T>& operator<<(const SenderID& id_)
                                    ^

{
static T msg;
const eCAL::experimental::measurement::base::DataTypeInformation datatype_info{
eCAL::message::GetTypeName(msg),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no member named 'message' in namespace 'eCAL' [clang-diagnostic-error]

        eCAL::message::GetTypeName(msg),
              ^

static T msg;
const eCAL::experimental::measurement::base::DataTypeInformation datatype_info{
eCAL::message::GetTypeName(msg),
eCAL::message::GetEncoding(msg),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no member named 'message' in namespace 'eCAL' [clang-diagnostic-error]

        eCAL::message::GetEncoding(msg),
              ^

@KerstinKeller KerstinKeller force-pushed the feature/msg-measurement branch from 400f940 to 398176b Compare March 27, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants