[core] split GetTopicNames into GetPublishedTopicNames and GetSubscribedTopicNames.
#1969
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| auto diff_time = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - start_time); | ||
| std::cout << "GetTopicsNames : " << static_cast<double>(diff_time.count()) / runs << " ms" << " (" << num_topics << " topics)" << std::endl; | ||
| std::cout << std::endl; | ||
| std::cout << "GetTopics : " << static_cast<double>(diff_time.count()) / runs << " ms" << " (" << num_entities << " entities)" << 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 << "GetTopics : " << static_cast<double>(diff_time.count()) / runs << " ms" << " (" << num_entities << " entities)" << std::endl; | |
| std::cout << "GetTopics : " << static_cast<double>(diff_time.count()) / runs << " ms" << " (" << num_entities << " entities)" << '\n'; |
| void GetSubscribedTopicNames(std::set<std::string>& topic_names_) | ||
| { | ||
| topic_names_.clear(); |
There was a problem hiding this comment.
Why take these as ref if you're going to wipe the input anyway? Are you assuming the implementation will reuse the previously allocated memory?
| void GetSubscribedTopicNames(std::set<std::string>& topic_names_) | |
| { | |
| topic_names_.clear(); | |
| std::set<std::string> GetSubscribedTopicNames() | |
| { | |
| std::set<std::string> topic_names_{}; |
There was a problem hiding this comment.
I'd be more than fine if they just returned the set. I guess we do have a few other functions which take a reference and then return bool.
Personally I don't like the style too much (I'd prefer something like std::expected, or std::variant, but since the core still uses C++ 14 that's out of the question 😞
Anyways, I guess the biggest problem here is that we're not consistent. ECAL_API std::set<SServiceId> GetClientIDs(); here we return a set, but ECAL_API bool GetClientInfo(const SServiceId& id_, ServiceMethodInformationSetT& service_method_info_); returns bool and takes a ref, and then lastly the functions to just get a name return void.
KerstinKeller
force-pushed
the
feature/registration-functions
branch
from
05b442a to
adbc6ef
Compare
KerstinKeller
force-pushed
the
feature/registration-functions
branch
from
adbc6ef to
2e41b7f
Compare
| const std::set<STopicId> pub_id_set = GetPublisherIDs(); | ||
| // get publisher id sets and insert names into the topic_names set | ||
| std::set<STopicId> pub_id_set; | ||
| bool return_value = GetPublisherIDs(pub_id_set); |
There was a problem hiding this comment.
warning: variable 'return_value' of type 'bool' can be declared 'const' [misc-const-correctness]
| bool return_value = GetPublisherIDs(pub_id_set); | |
| bool const return_value = GetPublisherIDs(pub_id_set); |
|
|
||
| // get subscriber id sets and insert names into the topic_names set | ||
| std::set<STopicId> sub_id_set; | ||
| bool return_value = GetSubscriberIDs(sub_id_set); |
There was a problem hiding this comment.
warning: variable 'return_value' of type 'bool' can be declared 'const' [misc-const-correctness]
| bool return_value = GetSubscriberIDs(sub_id_set); | |
| bool const return_value = GetSubscriberIDs(sub_id_set); |
| { | ||
| server_method_names_.clear(); | ||
|
|
||
| // get servers id set and insert names into the server_method_names_ set | ||
| const std::set<SServiceId> server_id_set = GetServerIDs(); | ||
| std::set<SServiceId> server_id_set; | ||
| bool return_value = GetServerIDs(server_id_set); |
There was a problem hiding this comment.
warning: variable 'return_value' of type 'bool' can be declared 'const' [misc-const-correctness]
| bool return_value = GetServerIDs(server_id_set); | |
| bool const return_value = GetServerIDs(server_id_set); |
| { | ||
| client_method_names_.clear(); | ||
|
|
||
| // get clients id set and insert names into the client_method_names set | ||
| const std::set<SServiceId> client_id_set = GetClientIDs(); | ||
| std::set<SServiceId> client_id_set; | ||
| bool return_value = GetClientIDs(client_id_set); |
There was a problem hiding this comment.
warning: variable 'return_value' of type 'bool' can be declared 'const' [misc-const-correctness]
| bool return_value = GetClientIDs(client_id_set); | |
| bool const return_value = GetClientIDs(client_id_set); |
| @@ -41,6 +41,8 @@ TEST(core_cpp_registration_public, GetTopics) | |||
|
|
|||
| std::set<eCAL::STopicId> topic_id_pub_set; | |||
| std::set<eCAL::STopicId> topic_id_sub_set; | |||
| bool get_publisher_ids_succeeded; | |||
There was a problem hiding this comment.
warning: variable 'get_publisher_ids_succeeded' is not initialized [cppcoreguidelines-init-variables]
| bool get_publisher_ids_succeeded; | |
| bool get_publisher_ids_succeeded = false; |
| @@ -41,6 +41,8 @@ TEST(core_cpp_registration_public, GetTopics) | |||
|
|
|||
| std::set<eCAL::STopicId> topic_id_pub_set; | |||
| std::set<eCAL::STopicId> topic_id_sub_set; | |||
| bool get_publisher_ids_succeeded; | |||
| bool get_subscriber_ids_succeeded; | |||
There was a problem hiding this comment.
warning: variable 'get_subscriber_ids_succeeded' is not initialized [cppcoreguidelines-init-variables]
| bool get_subscriber_ids_succeeded; | |
| bool get_subscriber_ids_succeeded = false; |
Please see if you like the naming.
To be more in line with the functions for service / client, the function was split into two separate functions.