Skip to content

Add unsigned specification to literals#5125

Closed
Blast545 wants to merge 2 commits intoeProsima:2.14.xfrom
Blast545:2.14.x
Closed

Add unsigned specification to literals#5125
Blast545 wants to merge 2 commits intoeProsima:2.14.xfrom
Blast545:2.14.x

Conversation

@Blast545
Copy link
Copy Markdown

@Blast545 Blast545 commented Aug 2, 2024 •
edited
Loading

Description

As the title says. The performance jobs in the OSRF buildfarm are getting a warning because the indexes used with the iHandle InstanceHandle_t are being forwarded to a template, and the compiler thinks that there may be lost data converting 0 and 15 from int to unsigned int.

Reference failing job: https://build.ros2.org/view/Rci/job/Rci__nightly-performance_ubuntu_noble_amd64/68/gcc/

Silly error, I think this PR should fix it.

I will test this PR in the buildfarm before asking for the PR to be merged.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@Blast545
Add unsigned specification to literal
5042fae 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@elianalf elianalf added this to the v2.14.4 milestone Aug 2, 2024
@JesusPoderoso
Copy link
Copy Markdown
Contributor

JesusPoderoso commented Aug 5, 2024

This fix should be present also in 3.x and 2.10.x.
What do you think @MiguelCompany?

@Blast545
Change type of iterator index
89b7945 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 marked this pull request as ready for review August 5, 2024 16:34
@Blast545
Copy link
Copy Markdown
Author

Blast545 commented Aug 5, 2024

It wasn't straightforward to test this one on our CI, so I went ahead and just tested locally.

I added an extra commit that I was missing: 89b7945

This is the job that we are aiming to get to green: https://build.ros2.org/view/Rci/job/Rci__nightly-performance_ubuntu_noble_amd64/68/

These are the warnings that we want to resolve by merging this PR: https://build.ros2.org/view/Rci/job/Rci__nightly-performance_ubuntu_noble_amd64/68/gcc/

And I'm attaching the output of my local environment for the performance_test repo in case that helps to check if the PR addresses the problem.

ros2_rolling/log/latest_build/performance_test/stdout_stderr.log

Let me know if I can do something else to help merging this, thank you!

@Blast545 Blast545 mentioned this pull request Aug 5, 2024
Merged
@Blast545
Copy link
Copy Markdown
Author

Blast545 commented Aug 7, 2024

PTAL 🙏

JesusPoderoso added a commit that referenced this pull request Aug 8, 2024
@JesusPoderoso
Refs #21470: Apply @Blast545 contribution (#5125)
690c97e 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@JesusPoderoso JesusPoderoso mentioned this pull request Aug 8, 2024
Merged
10 tasks
@JesusPoderoso
Copy link
Copy Markdown
Contributor

JesusPoderoso commented Aug 8, 2024

Hi @Blast545, thanks for your contribution.
I've opened a new PR with the changes introduced in the PR because our GitHub CI does not work properly with external contributions.
Therefore I am closing this PR.
Thanks!

MiguelCompany pushed a commit that referenced this pull request Aug 19, 2024
@JesusPoderoso
Add unsigned specification to literals (#5125) (#5149)
daef938 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
mergify bot pushed a commit that referenced this pull request Aug 19, 2024
@JesusPoderoso @mergify
Add unsigned specification to literals (#5125) (#5149)
9437a7d 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)
@mergify mergify bot mentioned this pull request Aug 19, 2024
Merged
10 tasks
MiguelCompany pushed a commit that referenced this pull request Aug 19, 2024
@mergify @JesusPoderoso
Add unsigned specification to literals (#5125) (#5149) (#5161)
44a6089 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Aug 19, 2024
@JesusPoderoso @mergify
Add unsigned specification to literals (#5125) (#5149)
390a5b0 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)
@mergify mergify bot mentioned this pull request Aug 19, 2024
Merged
10 tasks
MiguelCompany pushed a commit that referenced this pull request Aug 20, 2024
@mergify @JesusPoderoso
Add unsigned specification to literals (#5125) (#5149) (#5162)
bbb4c58 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Aug 26, 2024
@JesusPoderoso @mergify
Add unsigned specification to literals (#5125) (#5149)
b9ca5fc 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)

# Conflicts:
#	include/fastdds/rtps/common/InstanceHandle.h
@mergify mergify bot mentioned this pull request Aug 26, 2024
Merged
10 tasks
JesusPoderoso added a commit that referenced this pull request Aug 26, 2024
@JesusPoderoso
Add unsigned specification to literals (#5125) (#5149)
3ded683 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit daef938)
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
MiguelCompany pushed a commit that referenced this pull request Aug 26, 2024
@mergify @JesusPoderoso
Add unsigned specification to literals (#5125) (#5149) (#5174)
4c460ef 
(cherry picked from commit daef938)

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.14.4

Development

Successfully merging this pull request may close these issues.

3 participants

@Blast545 @JesusPoderoso @elianalf