Skip to content

open Node/Publisher API for allowing inheritance#258

Merged
Karsten1987 merged 5 commits intomasterfrom
lifecycle
Nov 1, 2016
Merged

open Node/Publisher API for allowing inheritance#258
Karsten1987 merged 5 commits intomasterfrom
lifecycle

Conversation

@Karsten1987
Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 commented Oct 26, 2016

change:

  • Node::create_publisher function has a new template parameter for publisher type (rclcpp::publisher::Publisher by default)
  • all publish functions are virtual for being able to inherit from

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Oct 26, 2016
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Oct 26, 2016

Node gets extended with one template parameter for publisher type

Isn't it more accurate to say that the Node::create_publisher method has a new template parameter? Node is still not templated as far as I can see.

@Karsten1987
Copy link
Copy Markdown
Contributor Author

You're absolutely right. Corrected.

@codebot codebot added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 1, 2016
typename rclcpp::publisher::Publisher<MessageT, Alloc>::SharedPtr
template<typename MessageT, typename Alloc = std::allocator<void>,
typename PublisherT = ::rclcpp::publisher::Publisher<MessageT, Alloc>>
std::shared_ptr<PublisherT>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I slightly different indentation would be nice. Not sure what our linters will be happy with, maybe:

template<
  typename MessageT, typename Alloc = std::allocator<void>,
  typename PublisherT = ::rclcpp::publisher::Publisher<MessageT, Alloc>>
std::shared_ptr<PublisherT>

@Karsten1987
Copy link
Copy Markdown
Contributor Author

updated

Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

lgtm

@Karsten1987
Copy link
Copy Markdown
Contributor Author

Karsten1987 commented Nov 1, 2016

build succesful http://ci.ros2.org/job/ci_linux/1873/

@Karsten1987 Karsten1987 merged commit 7f714a8 into master Nov 1, 2016
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Nov 1, 2016
@Karsten1987 Karsten1987 deleted the lifecycle branch November 1, 2016 22:08
@AravindaDP
Copy link
Copy Markdown

AravindaDP commented Nov 8, 2016

Probably we should open service client API also for inheritance?

I'm specifically interested in marking async_send_request call as virtual.

Note: What's I have in mind is for the unit testing purposes. (Mocking) Correct me if it is wrong way to handle things.

Karsten1987 added a commit that referenced this pull request Dec 7, 2016
* (dev) template create_publisher with publisher type

* (dev) template publisher type for dynamic publisher type instantiation

* (dev) make Publisher::publish function virtual

* (fix) uncrustify

* different indentation of long template declaration
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Feb 9, 2017

@AravindaDP sorry it took so long to get back to you on this. If you'd like to see additional methods marked virtual, or a function refactored to take one of the arguments as a template, please just open a pr to that effect or open an issue for each case so we can evaluate them there.

@AravindaDP
Copy link
Copy Markdown

@wjwwood no worries. Actually I wasn't sure about the correct place I should open the discussion.
I'll open an issue so we can evaluate the best approach to achieve the requirement there.

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
* Treat __name the same as __node

This modifies the lexer so that it will accept __name and treat it
exactly the same as __node.  __node still works in order to preserve
backwards compatibility.

Fixes ros2#258

Distribution Statement A; OPSEC ros2#2893

Signed-off-by: P. J. Reed <preed@swri.org>

* Update dot graph for lexer

Distribution Statement A; OPSEC ros2#2893

Signed-off-by: P. J. Reed <preed@swri.org>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* Add a SequentialCompressionReader

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Co-authored-by: Zachary Michaels <zmichaels11@users.noreply.github.com>
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.

5 participants