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

avoid unnecessary copy for rcutils_char_array_vsprintf. #412

Merged
merged 1 commit into from
Mar 2, 2023
Merged

avoid unnecessary copy for rcutils_char_array_vsprintf. #412

merged 1 commit into from
Mar 2, 2023

Conversation

fujitatomoya
Copy link
Collaborator

address #409

@fujitatomoya
avoid unnecessary copy for rcutils_char_array_vsprintf.
b09ef1a 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@clalancette minor improvement, can you review on this?

@fujitatomoya
Copy link
Collaborator Author

CC: @iuhilnehc-ynos

iuhilnehc-ynos
iuhilnehc-ynos reviewed Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As rcutils_char_array_vsprintf might call _rcutils_char_array_vsprintf twice, it seems necessary to use va_copy in _rcutils_char_array_vsprintf. This PR makes sense.

iuhilnehc-ynos
iuhilnehc-ynos approved these changes Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

LGTM

@ahcorde
Copy link
Contributor

ahcorde commented Mar 1, 2023

BUILD args: --packages-above-and-dependencies rcutils
TEST args: --packages-above rcutils
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11523

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

clalancette
clalancette requested changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

See my comment in ros2/rcl#1035 (review) for why I don't think this is safe to do.

@fujitatomoya
Copy link
Collaborator Author

https://ci.ros2.org/job/ci_linux/18129/testReport/ is known flaky addressed by ros2/ros2cli#809

@fujitatomoya fujitatomoya merged commit 1db1f16 into rolling Mar 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/20230301-rcutils_char_array_vsprintf branch March 2, 2023 16:20
@fujitatomoya
Copy link
Collaborator Author

@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Mar 2, 2023 •
edited
Loading

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Mar 2, 2023
@fujitatomoya @mergify
avoid unnecessary copy for rcutils_char_array_vsprintf. (#412)
687eeb2 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 1db1f16)
@mergify mergify bot mentioned this pull request Mar 2, 2023
Merged
fujitatomoya pushed a commit that referenced this pull request Mar 3, 2023
@mergify
avoid unnecessary copy for rcutils_char_array_vsprintf. (#412) (#413)
dee753a 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 1db1f16)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette requested changes

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@ahcorde ahcorde ahcorde approved these changes

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fujitatomoya @ahcorde @clalancette @iuhilnehc-ynos