Skip to content

Conversation

xinyazhang
Copy link
Contributor

@xinyazhang xinyazhang commented Nov 19, 2020

The coefficients of Eigen::Vector3d, like other data types in Eigen3,
are uninitialized by default.

This can be verified with the following code

#include <Eigen/Core>
#include <iostream>

int main() {
    {
        Eigen::Vector3d v;
        v << 1, 2, 3;
        std::cout << v(0) << " " << v(1) << " " << v(2) << std::endl;
    }
    {
        Eigen::Vector3d v;
        std::cout << v(0) << " " << v(1) << " " << v(2) << std::endl;
    }
    return 0;
}

This change is Reviewable

The coefficients of Eigen::Vector3d, like other data types in Eigen3,
are uninitialized by default.
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks for this fix. A few requests:

  • Please describe the problem this fixes. What happens if reference_p is uninitialized?
  • The problem exists in FCL because of the lack of unit tests in the original implementation. We have been following a policy of adding missing tests as we improve the code. Please add a unit test that would have failed with reference_p uninitialized and now succeeds because of your fix. That will prevent future programmers from breaking this code ever again.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @xinyazhang)

@xinyazhang
Copy link
Contributor Author

Thanks for this fix. A few requests:

* Please describe the problem this fixes. What happens if reference_p is uninitialized?

* The problem exists in FCL because of the lack of unit tests in the original implementation. We have been following a policy of adding missing tests as we improve the code. Please add a unit test that would have failed with reference_p uninitialized and now succeeds because of your fix. That will prevent future programmers from breaking this code ever again.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @xinyazhang)

Got it, will add unit tests later.

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.

2 participants