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

Make MangedBytes and VectorOf movable #1974

Merged
merged 3 commits into from
Jan 23, 2020
Merged

Make MangedBytes and VectorOf movable #1974

merged 3 commits into from
Jan 23, 2020

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Feb 26, 2019 •
edited
Loading

This PR makes ManagedBytes and VectorOf movable.

It fixes #1965.

Please review the code.

@Nicogene Nicogene added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_os Component: Library - YARP_sig PR Status: Changelog - OK This PR has a proper changelog entry Target: YARP v3.2.0 labels Feb 26, 2019
@Nicogene Nicogene requested a review from drdanz as a code owner February 26, 2019 15:58
@Nicogene Nicogene added the Type: Modernization Involves porting some part of code to modern C++ (11/14/17...) label Feb 26, 2019
@barbalberto
Copy link
Collaborator

Looks good, but can you add some unit tests?

@Nicogene
Copy link
Member Author

Looks good, but can you add some unit tests?

I don't think it is necessary, these operators are implicitly called in our unit test in place of some assignment operators.
In fact, in the previous version of this PR the tests crashed 😅

@Nicogene
Copy link
Member Author

BTW this PR is not ready, the Valgrind tests are failing.

@Nicogene Nicogene mentioned this pull request Mar 13, 2019
Closed
@Nicogene Nicogene force-pushed the makeVectorMovable branch from b60ebcd to 5e84713 Compare April 11, 2019 09:48
@Nicogene Nicogene mentioned this pull request Apr 12, 2019
Closed
@drdanz drdanz force-pushed the makeVectorMovable branch from 5e84713 to 98473c0 Compare April 18, 2019 06:24
@drdanz
Copy link
Member

drdanz commented Oct 7, 2019

@Nicogene what is the status of this PR? Are valgrind tests still failing? Do you mind rebasing this?

@Nicogene
Copy link
Member Author

Nicogene commented Oct 7, 2019

@drdanz if I remember correctly this PR introduces a valgrind error in the unit tests, I never been able to understand what was the problem.
If we want to invest time to make the yarp::sig::Vector movable we should investigate it, otherwise we can close this PR

@drdanz drdanz force-pushed the makeVectorMovable branch from 98473c0 to b426fba Compare January 21, 2020 15:26
@drdanz drdanz changed the base branch from devel to master January 21, 2020 15:27
@drdanz drdanz added Target: YARP v3.4.0 PR Status: Commits - Not OK Commits in this PR needs to be squashed or re-organized and removed Target: Future labels Jan 21, 2020
@drdanz
Copy link
Member

drdanz commented Jan 21, 2020

Rebased, changed base to master, fixed indentation, and, hopefully, fixed the memory leak.
@Nicogene The moveOwnership method was not freeing the memory before taking ownership of the new one (see 438bd5a).

If it works, I'll squash everything into your commit and finally merge this.

@drdanz drdanz force-pushed the makeVectorMovable branch from b426fba to 52aa9dc Compare January 22, 2020 14:16
@drdanz
Copy link
Member

drdanz commented Jan 22, 2020

I'm not sure why the CI was not available, I just forced it to start again

@pattacini pattacini removed their request for review January 22, 2020 14:45
@drdanz
Copy link
Member

drdanz commented Jan 23, 2020

image

I'll squash my commits and merge...

@drdanz drdanz force-pushed the makeVectorMovable branch from 52aa9dc to 63f490b Compare January 23, 2020 08:11
@drdanz drdanz merged commit ddb57bc into robotology:master Jan 23, 2020
@drdanz
Copy link
Member

drdanz commented Jan 23, 2020

Merged, thanks.

@drdanz drdanz added PR Status: Commits - OK Commits in this PR are OK Resolution: Merged and removed PR Status: Commits - Not OK Commits in this PR needs to be squashed or re-organized labels Jan 23, 2020
@Nicogene Nicogene mentioned this pull request Jan 23, 2020
Closed
@traversaro traversaro mentioned this pull request Jan 23, 2020
Closed
@Nicogene Nicogene deleted the makeVectorMovable branch March 3, 2021 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudiofantacci claudiofantacci claudiofantacci left review comments

@drdanz drdanz Awaiting requested review from drdanz

@aerydna aerydna Awaiting requested review from aerydna

@randaz81 randaz81 Awaiting requested review from randaz81

@traversaro traversaro Awaiting requested review from traversaro

@vtikha vtikha Awaiting requested review from vtikha

Assignees

@drdanz drdanz

@Nicogene Nicogene

Labels
Component: Library - YARP_os Component: Library - YARP_sig PR Status: Changelog - OK This PR has a proper changelog entry PR Status: Commits - OK Commits in this PR are OK PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.4.0 Type: Modernization Involves porting some part of code to modern C++ (11/14/17...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make yarp::sig::Vector movable
4 participants
@Nicogene @barbalberto @drdanz @claudiofantacci