Cleanup size and indices attributes, return values and arguments #767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In 4e23a1e I restored the template parameter of While this is probably an indication of some underlying issue in all the default define constructors (probably on how they handle const-correctness), handling this is way out of the scope of the issue, and so I will track this in a separate issue. |
traversaro
force-pushed
the
cleanupSizeAndIndeces
branch
from
3f03336
to
e31930b
Compare
traversaro
force-pushed
the
cleanupSizeAndIndeces
branch
from
e31930b
to
80b31d0
Compare
S-Dafarra
reviewed
Nov 18, 2020
Comment on lines
+77
to
+79
| std::stringstream ss; | ||
| ss << inInt; | ||
| return ss.str(); |
There was a problem hiding this comment.
There should be std::to_string
There was a problem hiding this comment.
I prefer to always avoid the use of std::to_string, as on integers it works fine, but for floating points create critical problems. However this is just adding a function that already existed for a different type, we can cleanup the function all together.
Since they are related to Eigen, isn't it because of the maps? |
S-Dafarra
approved these changes
Nov 18, 2020
It could be related, but rows and columns in iDynTree were unsigned before, and now they are std::size_t (so always unsigned). Similarly, the template parameters were updated from unsigned to std::size_t, so always unsigned. I do not know why changing the template parameter from unsigned to std::size_t triggers that problems, and probably it is indeed related to Eigen::Map, but I don't know immediately if it is related to the fact that rows and columns use signed return values. |
This was referenced Nov 18, 2020
francesco-romano
added a commit
to francesco-romano/idyntree
that referenced
this pull request
Nov 24, 2020
robotology#767 changed the index type but did not update the bindings code accordingly.
francesco-romano
added a commit
to francesco-romano/idyntree
that referenced
this pull request
Nov 24, 2020
robotology#767 changed the index type but did not update the bindings code accordingly.
traversaro
added a commit
that referenced
this pull request
Nov 24, 2020
Fix pybind11 bindings compilation after #767.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It just started for solving some warnings:
And as usual it escalated to a large scale change. However, it is better to do this before the iDynTree 2 releases as it changes (slightly) the API and the ABI.
For consistency with std and Eigen, all sizes and indices have been changed to use
std::size_tfor unsigned quantities andstd::ptrdiff_tfor signed quantities. The only exception is the index stored in the triplets of the iDynTree::SparseMatrix data structure, that have been left defined tointfor compatibility with Eigen.