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 TrajkovicKeypoint2D/3D to CMake build #2179

Merged
merged 2 commits into from
Jan 12, 2018
Merged

Add TrajkovicKeypoint2D/3D to CMake build #2179

merged 2 commits into from
Jan 12, 2018

Conversation

ThorstenHarter
Copy link
Contributor

The files which have been added to PCL in #409 are missing in the CMakeLists.txt of the keypoints module.

Fix Microsoft Visual Studio 2015 compile error: "trajkovic_3d.hpp(235): error C3016: 'i': index variable in OpenMP 'for' statement must have signed integral type" by changing index type from size_t to int.

The files which have been added to PCL in #409 are missing in the CMakeLists.txt of the keypoints module.
Fix Microsoft Visual Studio 2015 compile error: "trajkovic_3d.hpp(235): error C3016: 'i': index variable in OpenMP 'for' statement must have signed integral type"
@ThorstenHarter ThorstenHarter mentioned this pull request Jan 10, 2018
@SergioRAgostinho
Copy link
Member

Have a look at

https://github.com/PointCloudLibrary/pcl/blob/master/common/include/pcl/pcl_macros.h#L44-L55
http://www.boost.org/doc/libs/1_55_0/boost/cstdint.hpp

The boost provided intmax_t might be the safest option in this scenario.

@ThorstenHarter
Copy link
Contributor Author

You mean like this?

  for (intmax_t i = 0; i < indices.size (); ++i)
  {
    int idx = indices[i];

I automatically added the casts because we use -Wsign-conversion in our projects, but since this isn't activated in PCL, they are not mandatory.

@SergioRAgostinho
Copy link
Member

You still need to cast indices.size() like you did before because of what of Wsign-comparison. It is active in PCL for clang (we do need to revise/unify our flags across different compilers). So please suppress all of those.

The point behind my comment was to make sure we use the highest range integer possible, that's available on each platform, to avoid potential overflows on very big indices vectors. int didn't ensure that but intmax_t seems to.

@ThorstenHarter
Copy link
Contributor Author

So you're saying a PointCloud could have more than INT_MAX points?
Because only then the indices vector of the keypoints search could be bigger than that.

In that case the filters, features and keypoints algorithms won't work anyway, because the indices are always defined like this: std::vector<int> indices

In other words, I don't see the sense in changing the type of 'i' to intmax_t, while 'idx', 'indices' etc. are using the type int.

@SergioRAgostinho
Copy link
Member

That's a valid point. I regret a little what our "big data" users deal with. Hopefully someone with those needs will have a look at things and decide to push some changes.

@SergioRAgostinho SergioRAgostinho merged commit 075539d into PointCloudLibrary:master Jan 12, 2018
@SergioRAgostinho SergioRAgostinho changed the title Add TrajkovicKeypoint2D/3D to CMake build Add TrajkovicKeypoint2D/3D to CMake build Aug 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants