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.

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

Add begin/end member functions for iDynTree vectors #646

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

GiulioRomualdi
Copy link
Member

This PR:

  • Implement begin()/end() and cbegin()/cend() methods for VectorFixSize
  • Implement begin()/end() and cbegin()/cend() methods for VectorDynSize
  • Update the CHANGELOG.md

The implementation of these members will allow the user to use range-based for loop.

Related issue #645

@traversaro
Copy link
Member

Thanks for the contribution @GiulioRomualdi ! Technically this is a (small) new feature, so I would prefer to merge it in devel and eventually release it as iDynTree 1.1 , instead of merging it in master and release it as part of iDynTree 1.0.2, to comply with SemVer (see https://semver.org/).

However, as it is the "saturday that is made for the men, not the men for the saturday", i.e. we follow guidelines only if we gain advantage from them, not because they are valuable on its own, if you have any reason why you want this in master let me know, thanks!

@GiulioRomualdi
Copy link
Member Author

Hi @traversaro. Devel is fine!

@traversaro
Copy link
Member

Just to clarify, now that I cleaned up a bit the release process of iDynTree, I think if we need a iDynTree 1.1 we can easily release it when we need it, without waiting years. : )

@traversaro
Copy link
Member

Hi @traversaro. Devel is fine!

Perfect, then feel free to change the target branch.

@GiulioRomualdi GiulioRomualdi changed the base branch from master to devel January 28, 2020 09:52
@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Jan 28, 2020

Rebased on top of devel

@GiulioRomualdi
Copy link
Member Author

The spurious commits are there because master is not merged in devel
image

@DanielePucci
Copy link
Collaborator

As for future reference,

"saturday that is made for the men, not the men for the saturday"

it is Gospel of Mark, 2:27. I had to google for it, I believed it was actually Gospel of Mathew.

CC
@traversaro @GiulioRomualdi

@traversaro
Copy link
Member

The spurious commits are there because master is not merged in devel
image

Sorry, I fixed it now.

@GiulioRomualdi
Copy link
Member Author

The spurious commits are there because master is not merged in devel
image

Sorry, I fixed it now.

Rebased on devel. You can merge

@traversaro
Copy link
Member

Thanks!

@traversaro traversaro merged commit 4427d95 into robotology:devel Jan 29, 2020
@S-Dafarra
Copy link
Contributor

Sorry for the late comment. Was there a reason not to use an std::iterator? I am not a great fan of using a pointer to double as iterator 🤔

@traversaro
Copy link
Member

Sorry for the late comment. Was there a reason not to use an std::iterator? I am not a great fan of using a pointer to double as iterator 🤔

What do you think @GiulioRomualdi ? Until we actually release iDynTree 1.1, we can change the interface any time we want, so feel free to propose a change if you see fit.

@GiulioRomualdi
Copy link
Member Author

std::iterator has been deprecated in C++17, that's the reason why I didn't use it. I don't know which is the alternative :sad:

@S-Dafarra
Copy link
Contributor

To be honest I did not know. Apparently the alternative is simply to define your own iterator without inheriting from std::iterator, but defining all the aliases (https://www.fluentcpp.com/2018/05/08/std-iterator-deprecated/).

@GiulioRomualdi GiulioRomualdi deleted the feature/begin_end_methods branch March 2, 2020 23:00
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.

4 participants