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

Removed warning #4693

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Removed warning #4693

merged 2 commits into from
Apr 18, 2024

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Apr 17, 2024

There is a new warning on CI https://ci.ros2.org/view/packaging/job/packaging_linux/3405/clang-tidy/folder.112366245/

This should fix the warning

@ahcorde
Removed warning
e59a13d 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
MiguelCompany
MiguelCompany requested changes Apr 17, 2024
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

I would also change the order of the comparisons in line 149 to first check for valid, and then check the pid

        if (!valid || (pid == PID_SENTINEL))

@ahcorde
Added suggestions
4785b26 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from MiguelCompany April 17, 2024 12:18
@MiguelCompany MiguelCompany added the ci-pending PR which CI is running label Apr 17, 2024
@MiguelCompany MiguelCompany self-requested a review April 17, 2024 12:58
@MiguelCompany MiguelCompany added this to the v2.14.1 milestone Apr 17, 2024
@MiguelCompany
Copy link
Member

@richiprosima Please test this

@JesusPoderoso
Copy link
Contributor

It seems CI issues are unrelated to the PR.
Labeling it as ready to merge

@JesusPoderoso JesusPoderoso added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Apr 18, 2024
@MiguelCompany MiguelCompany merged commit 8258507 into eProsima:master Apr 18, 2024
9 of 26 checks passed
@MiguelCompany
Copy link
Member

It seems CI issues are unrelated to the PR. Labeling it as ready to merge

I should have labeled this as no-test.

Thanks for this @ahcorde !

@MiguelCompany
Copy link
Member

@Mergifyio backport 2.13.x 2.10.x 2.6.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 18, 2024 •
edited
Loading

backport 2.13.x 2.10.x 2.6.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 18, 2024
@ahcorde @mergify
Removed warning (#4693)
a1b0a62 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)
mergify bot pushed a commit that referenced this pull request Apr 18, 2024
@ahcorde @mergify
Removed warning (#4693)
07ae7d6 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)
This was referenced Apr 18, 2024
Merged
Merged
mergify bot pushed a commit that referenced this pull request Apr 18, 2024
@ahcorde @mergify
Removed warning (#4693)
3886888 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)
@mergify mergify bot mentioned this pull request Apr 18, 2024
Merged
EduPonz pushed a commit that referenced this pull request Apr 18, 2024
@mergify @ahcorde
Removed warning (#4693) (#4702)
47a7691 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
MiguelCompany pushed a commit that referenced this pull request Apr 19, 2024
@mergify @ahcorde
Removed warning (#4693) (#4703)
3c95230 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
MiguelCompany pushed a commit that referenced this pull request Apr 23, 2024
@mergify @ahcorde
Removed warning (#4693) (#4704)
eb52759 
* Removed warning

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

* Added suggestions

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

---------

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit 8258507)

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany Awaiting requested review from MiguelCompany

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.14.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @MiguelCompany @JesusPoderoso