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

first draft of a new client api #1785

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rex-schilasky
Copy link
Contributor

Description

THIS IS JUST A WORKING DRAFT FOR A NEW CLIENT API.

@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Nov 6, 2024
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

/**
* @brief Service client wrapper class.
**/
class CServiceClientID
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'CServiceClientID' defines a non-default destructor, a copy constructor and a copy assignment operator but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

  class CServiceClientID
        ^


void ApplyServiceRegistration(const Registration::Sample& ecal_sample_);

void GetRegistrations(Registration::SampleList& reg_sample_list_);

protected:
static std::atomic<bool> m_created;
static std::atomic<bool> m_created;
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 'm_created' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

    static std::atomic<bool>      m_created;
                                  ^

bool CServiceClientID::AddEventCallback(eCAL_Client_Event type_, ClientEventCallbackT callback_)
{
if (!m_service_client_impl) return false;
return m_service_client_impl->AddEventCallback(type_, callback_);
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 'callback_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

ecal/core/src/service/ecal_service_client_id.cpp:25:

+ #include <utility>
Suggested change
return m_service_client_impl->AddEventCallback(type_, callback_);
return m_service_client_impl->AddEventCallback(type_, std::move(callback_));

}

// Create a response callback, that will set the response and notify the condition variable
eCAL::service::ClientResponseCallbackT response_callback
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 'response_callback' of type 'eCAL::service::ClientResponseCallbackT' (aka 'function<void (const eCAL::service::Error &, const shared_ptr<basic_string> &)>') can be declared 'const' [misc-const-correctness]

Suggested change
eCAL::service::ClientResponseCallbackT response_callback
eCAL::service::ClientResponseCallbackT const response_callback

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 24. Check the log or trigger a new build to see more.

void CServiceClientIDImpl::ResetAllCallbacks()
{
{
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
std::lock_guard<std::mutex> const lock(m_client_session_map_sync);

m_client_session_map.clear();
}
{
std::lock_guard<std::mutex> lock(m_response_callback_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_response_callback_sync);
std::lock_guard<std::mutex> const lock(m_response_callback_sync);

m_response_callback = nullptr;
}
{
std::lock_guard<std::mutex> lock(m_event_callback_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_event_callback_map_sync);
std::lock_guard<std::mutex> const lock(m_event_callback_map_sync);

// Adds a callback function for service responses
bool CServiceClientIDImpl::AddResponseCallback(const ResponseIDCallbackT& callback_)
{
std::lock_guard<std::mutex> lock(m_response_callback_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_response_callback_sync);
std::lock_guard<std::mutex> const lock(m_response_callback_sync);

// Removes the service response callback
bool CServiceClientIDImpl::RemoveResponseCallback()
{
std::lock_guard<std::mutex> lock(m_response_callback_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_response_callback_sync);
std::lock_guard<std::mutex> const lock(m_response_callback_sync);

};
}

SServiceResponse CServiceClientIDImpl::PrepareErrorResponse(const std::string& error_message)
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 'PrepareErrorResponse' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/service/ecal_service_client_id_impl.h:163:

-     SServiceResponse PrepareErrorResponse(const std::string& error_message);
+     static SServiceResponse PrepareErrorResponse(const std::string& error_message);


void CServiceClientIDImpl::IncrementMethodCallCount(const std::string& method_name_)
{
std::lock_guard<std::mutex> lock(m_method_information_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_method_information_map_sync);
std::lock_guard<std::mutex> const lock(m_method_information_map_sync);

// Check if a specific service is connected
bool CServiceClientIDImpl::IsConnected(const Registration::SEntityId& entity_id_)
{
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
std::lock_guard<std::mutex> const lock(m_client_session_map_sync);

// Check if any service is connected
bool CServiceClientIDImpl::IsConnected()
{
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
std::lock_guard<std::mutex> const lock(m_client_session_map_sync);


void CServiceClientIDImpl::RegisterService(const Registration::SEntityId& entity_id_, const SServiceAttr& service_)
{
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
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 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]

Suggested change
std::lock_guard<std::mutex> lock(m_client_session_map_sync);
std::lock_guard<std::mutex> const lock(m_client_session_map_sync);

@rex-schilasky rex-schilasky changed the title first draft of a new client api (not implemented, not compiling) first draft of a new client api Nov 12, 2024
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.

1 participant