Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement generic client #2358
Implement generic client #2358
Changes from 6 commits
beeadfc
a1fca1a
5906087
8ef18fe
f3933cf
8540fe6
a73f5ed
da86048
69145c4
6f0cec6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nitpick: if you need to multi-line the function prototypes, I think it's cleaner to put the return code on its own line first, then the arguments, and if you do that for one function, it's helpful for readability (again in my opinion) to put them all on their own line even for functions that are short. This squarely falls into developer discretion, but I wanted to point it out. To be clear, I'm suggesting:
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.
I will update other declarations to use this coding 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.
Copy and pasting large chunks of logic like this one (pulled from
rclcpp/rclcpp/include/rclcpp/client.hpp
Lines 761 to 787 in 126d517
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.
@wjwwood
"Currently, pending_requests_ is defined in GenericClient class and Client class. The operations on pending_requests_ involve duplicated code.
There are two ways to avoid duplicated codes:
Move pending_requests_ and the corresponding mutex lock into the client base class. Since pending_requests_ contains different contents, the BaseClient class must be changed to a template class. This change would be quite significant.
Use macros to include the same code and place the macros in client.hpp."
I am inclined towards 2.
I would like to hear your opinion.
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.
Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.
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.
Yes. Using function is okay.
pending_requests_
must be passed, and the type ofpending_requests_
is different for Client and GenericClient. So the function have to be a template function.