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

Vector typedef of VectorOf<double> #1657

Merged
merged 11 commits into from
Jul 10, 2018
Merged

Vector typedef of VectorOf<double> #1657

merged 11 commits into from
Jul 10, 2018

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Apr 23, 2018 •
edited
Loading

This PR refactors yarp::sig::Vector in order to be a typedef of yarp::sig::VectorOf<double>

Moreover it enables and fixes the VectorOfTest disabled because broken.

I have to check how these changes impact the build on windows.

Fixes #1598

please review code.

@Nicogene Nicogene added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_sig Component: Tests Type: Cleanup Involves cleaning up some part of YARP PR Status: Changelog - OK This PR has a proper changelog entry Target: YARP v3.0.0 labels Apr 23, 2018
@Nicogene Nicogene self-assigned this Apr 23, 2018
@pattacini
Copy link
Member

@Nicogene be considerate and run tests thoroughly before merging this!

@Nicogene
Copy link
Member Author

Yes sure! If you know any other test I can do for testing this, please let me know 😉

@Nicogene Nicogene force-pushed the vectorRefactoring branch from 7f4208c to 35d8a52 Compare April 23, 2018 16:13
@Nicogene Nicogene mentioned this pull request Apr 24, 2018
Closed
@Nicogene Nicogene added the PR Status: Test on Hardware - Not OK The code in this PR needs to be tested on the robot setup label Apr 26, 2018
@Nicogene Nicogene force-pushed the vectorRefactoring branch from 8ef301b to 758848a Compare April 27, 2018 08:31
@Nicogene
Copy link
Member Author

As soon as I have all green lights I will test the redball demo on the robot, should be enough ?

@pattacini
Copy link
Member

@Nicogene, better rather run the simulated red-ball demo using RTF and then go on the real robot.

@pattacini
Copy link
Member

This is the test I was mentioning: https://github.com/robotology/icub-tests/tree/master/src/demoRedBall.

@Nicogene
Copy link
Member Author

Nicogene commented May 2, 2018

Sucessfully tested with the demoRedBallTest. Waiting to test it on the real robot.

@Nicogene Nicogene force-pushed the vectorRefactoring branch 2 times, most recently from d4d8fa6 to b61aa56 Compare May 4, 2018 16:10
@Nicogene Nicogene changed the title [WIP] Vector typedef of VectorOf<double> Vector typedef of VectorOf<double> May 10, 2018
@Nicogene Nicogene force-pushed the vectorRefactoring branch 2 times, most recently from 00c8177 to d722c74 Compare May 16, 2018 15:15
@Nicogene Nicogene added PR Status: Test on Hardware - OK The code in this PR was successfully tested on the robot setup and removed PR Status: Test on Hardware - Not OK The code in this PR needs to be tested on the robot setup labels May 16, 2018
@Nicogene Nicogene force-pushed the vectorRefactoring branch from d722c74 to aab912e Compare May 22, 2018 15:25
@Nicogene
Copy link
Member Author

@drdanz rebased on latest devel, ready for the review

@Nicogene Nicogene force-pushed the vectorRefactoring branch from f49984c to 554fe06 Compare June 20, 2018 09:51
@Nicogene Nicogene force-pushed the vectorRefactoring branch from 554fe06 to 3b90d96 Compare June 20, 2018 09:57
@Nicogene
Copy link
Member Author

Nicogene commented Jul 9, 2018 •
edited
Loading

Thanks to #1773 I figured out that this PR breaks the bindings, so DO NOT MERGE.
Swig doesn't like typedef VectorOf<double> Vector; even with the explicit instantiation of the template.. Any idea?

@traversaro
Copy link
Member

@Nicogene did you used the %template SWIG command?

@Nicogene Nicogene force-pushed the vectorRefactoring branch from 3b90d96 to 6dc6741 Compare July 10, 2018 08:58
@Nicogene
Copy link
Member Author

Thanks @traversaro , now it should be ok, I had to add #ifndef SWIG #endif around the typedef because SWIG was crashing generating the ruby bindings.
Probably it is a SWIG bug we should investigate it with a smaller example.

drdanz
drdanz approved these changes Jul 10, 2018
View reviewed changes
Copy link
Member

@drdanz drdanz left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment

src/libYARP_sig/src/Vector.cpp Outdated
@@ -41,6 +41,14 @@ class VectorPortContentHeader
};
YARP_END_PACK

const std::map<int, std::string> tag2FormatStr = {
{BOTTLE_TAG_INT32, "d"},
{BOTTLE_TAG_INT64, "ld"},
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to use PRId32 and PRId64 here

Nicogene added 3 commits July 10, 2018 13:33
@Nicogene
VectorOf: fixes after deprecation of BOTTLE_TAG_DOUBLE/INT and const …
f248242 
…correctness
@Nicogene
Vector: fixes after deprecation of ConstString
13a9bd4
@Nicogene
Vector: fix bindings after making Vector a typedef of VectorOf
2417c7d 
Swig(3.0.12) crashes when generating ruby bindings without these guards.
Bindings for Vector are generated anyways throught the %template directive
in the interface file.
@Nicogene Nicogene force-pushed the vectorRefactoring branch from 6dc6741 to 2417c7d Compare July 10, 2018 11:36
@Nicogene Nicogene merged commit f5bfc9b into robotology:devel Jul 10, 2018
@Nicogene Nicogene deleted the vectorRefactoring branch July 10, 2018 12:23
@Nicogene Nicogene mentioned this pull request Jul 10, 2018
Closed
@nunoguedelha nunoguedelha mentioned this pull request Jul 29, 2018
Merged
nunoguedelha added a commit to robotology/idyntree that referenced this pull request Jul 29, 2018
@nunoguedelha
Fixed Travis failing because of conflicting forward declaration of cl…
ba4764e 
…ass yarp::sig::Vector

This is related to robotology/yarp#1657 which refactored the class `Vector` to be a typedef of `VectorOF<double>`.
traversaro pushed a commit to robotology/idyntree that referenced this pull request Jul 31, 2018
@nunoguedelha @traversaro
Fixed Travis failing because of conflicting forward declaration of cl…
2766360 
…ass yarp::sig::Vector

This is related to robotology/yarp#1657 which refactored the class `Vector` to be a typedef of `VectorOF<double>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drdanz drdanz drdanz approved these changes

@traversaro traversaro Awaiting requested review from traversaro

@pattacini pattacini Awaiting requested review from pattacini

@barbalberto barbalberto Awaiting requested review from barbalberto

Assignees

@Nicogene Nicogene

Labels
Component: Library - YARP_sig Component: Tests PR Status: Changelog - OK This PR has a proper changelog entry PR Status: Test on Hardware - OK The code in this PR was successfully tested on the robot setup PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.1.0 Type: Cleanup Involves cleaning up some part of YARP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nicogene @pattacini @traversaro @drdanz @aerydna