Skip to content

chore: remove outdated TODO in GenericClient#3146

Merged
jmachowinski merged 2 commits into
ros2:rollingfrom
Gelminaio:patch-3
May 13, 2026
Merged

chore: remove outdated TODO in GenericClient#3146
jmachowinski merged 2 commits into
ros2:rollingfrom
Gelminaio:patch-3

Conversation

@Gelminaio
Copy link
Copy Markdown
Contributor

@Gelminaio Gelminaio commented May 12, 2026

Description

Removed an incorrect TODO comment

Is this user-facing behavior change?

No

Did you use Generative AI?

No

Additional Information

…icClient

Signed-off-by: Pietro Gelmini <86184562+Gelminaio@users.noreply.github.com>
@jmachowinski
Copy link
Copy Markdown
Collaborator

The TODO is just plain wrong. We should not use the C allocator here.

@jmachowinski
Copy link
Copy Markdown
Collaborator

Also, are you sure you did not use AI ?

@Gelminaio
Copy link
Copy Markdown
Contributor Author

Yeah, you got me 😅. To be completely honest, I used Gemini to help me with the exact syntax for the allocator, since I'm still getting used to the ROS 2 memory management internals. I didn't check the AI box because I thought it was meant for fully written by AI, not just snippets. Completely my bad! Good to know that the TODO is just outdated. I'll close this PR right away. Sorry for the noise!

@Gelminaio Gelminaio closed this May 12, 2026
@jmachowinski
Copy link
Copy Markdown
Collaborator

You can modify the PR to remove the todo though...

@Gelminaio Gelminaio reopened this May 13, 2026
Removed custom allocator for request header and used new allocation method.

Signed-off-by: Pietro Gelmini <86184562+Gelminaio@users.noreply.github.com>
@Gelminaio Gelminaio changed the title refactor: resolve TODO to use allocator for rmw_request_id_t in Gener… chore: remove outdated TODO in GenericClient May 13, 2026
@Gelminaio
Copy link
Copy Markdown
Contributor Author

Removed the TODO and kept the original code as suggested. Thanks for the catch! @jmachowinski

@jmachowinski
Copy link
Copy Markdown
Collaborator

Merging without CI, as only comments were modified.

@jmachowinski jmachowinski merged commit af9d1d4 into ros2:rolling May 13, 2026
3 checks passed
@christophebedard
Copy link
Copy Markdown
Member

Merging without CI, as only comments were modified.

I think the process is to skip ci.ros2.org when comments are modified, but we do still want to have a green *pr job (e.g., Rpr for Rolling) in case a linters complain, e.g., line length. In this case, *pr jobs are disabled, so a simple (maybe Linux-only) ci.ros2.org would've been good.

@Gelminaio Gelminaio deleted the patch-3 branch May 20, 2026 17:10
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.

3 participants