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.

Sign up for GitHub

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

Jump to bottom

Made options optional. Updated API documentation. #775

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Made options optional. Updated API documentation. #775

merged 1 commit into from
Jul 2, 2019

Conversation

esteve
Copy link
Member

@esteve esteve commented Jul 2, 2019

This PR updates the constructor to match that of Node (i.e. options should be optional). I also removed part of the API documentation that was no longer relevant.

@esteve
Made options optional. Updated API documentation.
b3f1ba8 
Signed-off-by: Esteve Fernandez <esteve@apache.org>
@esteve esteve requested a review from Karsten1987 July 2, 2019 10:53
ivanpauno
ivanpauno approved these changes Jul 2, 2019
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno
Copy link
Member

CI (up to test_rclcpp rclcpp, fastrtps only)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 3786578 into ros2:master Jul 2, 2019
@ivanpauno ivanpauno added this to Proposed in Dashing Patch Release 2 via automation Jul 2, 2019
@esteve esteve deleted the node_options_optional branch July 2, 2019 15:08
@ivanpauno ivanpauno moved this from Proposed to Needs Backport in Dashing Patch Release 2 Jul 3, 2019
@dirk-thomas
Copy link
Member

The current change is not ABI compatible and can therefore not be backported to Dashing as-is.

@ivanpauno was this selected for a specific use case to be backported? The simple workaround seems to be to just pass the default value explicitly.

@esteve @ivanpauno If either of you still thinks this should be backported can you please create a custom PR targeting the dashing branch which adds a new constructor without the options argument which calls the existing constructor with the explicit default value.

@sloretz
Copy link
Contributor

sloretz commented Jul 25, 2019 •
edited
Loading

The current change is not ABI compatible and can therefore not be backported to Dashing as-is.

@dirk-thomas How does this break ABI? It looks like the constructor has the same number of arguments with the same types as before.

@ivanpauno
Copy link
Member

I think that usually defaults are implemented as temporary objects created at the point of call, and it calls the same symbol when using the default than when passing the parameter explicitly.
Compilers may create two symbols in this case, one for the method taking the default and one for the non default.

In the first case this would be ABI compatible, in the second not.
I think that most compilers do the first thing, but I can't confirm that.

For me, it's okay not doing a backport.

@dirk-thomas
Copy link
Member

I couldn't come up with a reference. With g++ I can confirm that the symbol stays the same so for that compiler the change is ABI compatible.

In the first case this would be ABI compatible, in the second not.

The second case would add a new (non-virtual) constructor with one argument less. That change should be ABI compatible too. I will take back my objection about the ABI compatibility then.

The other question remains though: why should this be backported?

@ivanpauno
Copy link
Member

The other question remains though: why should this be backported?

There's not good reason, so I think not doing the backport is fine.
I'm deleting it from the project board.

@ivanpauno ivanpauno removed this from Needs Backport in Dashing Patch Release 2 Jul 25, 2019
@esteve
Copy link
Member Author

esteve commented Jul 26, 2019

@dirk-thomas The other question remains though: why should this be backported?

I don't really see why not, it'd make the lifecycle API consistent with the node API, it's not breaking the ABI and also fixes the documentation.

dirk-thomas pushed a commit that referenced this pull request Jul 26, 2019
@esteve @dirk-thomas
Add default value to options in LifecycleNode construnctor. Update AP…
eed46ac 
…I documentation. (#775)

Signed-off-by: Esteve Fernandez <esteve@apache.org>
@dirk-thomas dirk-thomas added this to Proposed in Dashing Patch Release 2 via automation Jul 26, 2019
@dirk-thomas
Copy link
Member

Since there is another backport for this repo I don't mind to also pick this one, see #801.

dirk-thomas added a commit that referenced this pull request Jul 26, 2019
@dirk-thomas
Add default value to options in LifecycleNode construnctor. Update AP…
6d3cbd3 
…I documentation. (#775) (#801)

Signed-off-by: Esteve Fernandez <esteve@apache.org>
@dirk-thomas dirk-thomas moved this from Proposed to Needs Release in Dashing Patch Release 2 Jul 26, 2019
@wjwwood wjwwood moved this from Needs Release to Released in Dashing Patch Release 2 Aug 1, 2019
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@brawner
Don't overwrite string_array pointer on reallocation failure (ros2#775)
d716ad1 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@MichaelOrlov
Add callbacks for PlayerClock::jump(time_point) API (ros2#775)
abd4229 
* Add callbacks to the jump API

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add unit tests for TimeControllerClock::jump callbacks

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Address some code style issues and renames

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Change Doxygen comments to `C` style.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Allow one of the callbacks to be nullptr.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@emersonknapp
Revert "Add callbacks for PlayerClock::jump(time_point) API (ros2#775)…
758d96b 
…" (ros2#778)

This reverts commit abd4229.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Dashing Patch Release 2
Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@esteve @ivanpauno @dirk-thomas @sloretz