Skip to content

accept custom allocator for LoanedMessage.#2672

Merged
fujitatomoya merged 2 commits intorollingfrom
fujitatomoya/loaned-message-custom-allocator
Nov 21, 2024
Merged

accept custom allocator for LoanedMessage.#2672
fujitatomoya merged 2 commits intorollingfrom
fujitatomoya/loaned-message-custom-allocator

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented Nov 13, 2024

closes #2671

@fujitatomoya
accept custom allocator for LoanedMessage.
6fb448e 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
fujitatomoya
fujitatomoya commented Nov 13, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@thomasmoore-torc could you review this?

@fujitatomoya fujitatomoya mentioned this pull request Nov 13, 2024
Closed
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 13, 2024

Pulls: #2672
Gist: https://gist.githubusercontent.com/fujitatomoya/c94c33d5d9b178b7fdb3a880191237b7/raw/ae815ef620a2b5f805d36af338047dadb02555dd/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14815

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

thomasmoore-torc
thomasmoore-torc approved these changes Nov 14, 2024
View reviewed changes
Copy link
Copy Markdown

@thomasmoore-torc thomasmoore-torc left a comment

Choose a reason for hiding this comment

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

Would it make sense to make MessageAllocatorTraits and MessageAllocator public instead of private? I don't think it's necessary for this specific change, but it would make LoanedMessage be more consistent with other class definitions in rclcpp.

Also, would it be possible to back-port this change to jazzy and iron?

@fujitatomoya
move message allocator using directives to public.
f3d5568 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 18, 2024

Also, would it be possible to back-port this change to jazzy and iron?

I think we can, this can be more flexible to take the argument for message allocator, besides no tests are broken with this change.
but technically this breaks ABI compatibility, that is what i am not fully sure.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 19, 2024

Pulls: #2672
Gist: https://gist.githubusercontent.com/fujitatomoya/9fe827a2c4aba0b645be3e8916f1b248/raw/ae815ef620a2b5f805d36af338047dadb02555dd/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14835

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

@fujitatomoya fujitatomoya merged commit 7e9ff5f into rolling Nov 21, 2024
HarunTeper pushed a commit to HarunTeper/rclcpp that referenced this pull request Dec 9, 2024
@fujitatomoya @HarunTeper
accept custom allocator for LoanedMessage. (ros2#2672)
574308b 
* accept custom allocator for LoanedMessage.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* move message allocator using directives to public.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

---------

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: HarunTeper <harun.teper@tu-dortmund.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

+1 more reviewer

@thomasmoore-torc thomasmoore-torc thomasmoore-torc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unable to use custom allocator with LoanedMessage

3 participants

@fujitatomoya @ahcorde @thomasmoore-torc