Skip to content

Fix OpenMP compile issue under MSVC#5453

Merged
mvieth merged 1 commit intoPointCloudLibrary:masterfrom
SunBlack:fix_openmp_msvc
Oct 6, 2022
Merged

Fix OpenMP compile issue under MSVC#5453
mvieth merged 1 commit intoPointCloudLibrary:masterfrom
SunBlack:fix_openmp_msvc

Conversation

@SunBlack
Copy link
Copy Markdown
Contributor

@SunBlack SunBlack commented Oct 5, 2022

Fix error C3016: 'i': index variable in OpenMP 'for' statement must have signed integral type

Issue occurs, as MSVC doesn't support size_t as OpenMP loop variable (see here).

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Oct 5, 2022

Needs a cast around size().
Do you know why this error didn't appear earlier? Do we not compile with openmp on the CI checks?
As a note: some other classes use std::ptrdiff_t for this purpose (but I am also fine with long long).

@SunBlack
Copy link
Copy Markdown
Contributor Author

SunBlack commented Oct 5, 2022

Thx for the hint with std::ptrdiff_t 👍

The compile issues is not happening on the CI, as there are no tests for TrajkovicKeypoint2D. The app pcl_ni_trajkovic (which requires OpenNI(1), which cannot be installed via vcpkg) is the only file which creates an instance of this class (as it is an template, there is no reason for the compiler to precompile it).

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Oct 6, 2022

Ah, I see. Thanks for the explanation.
However, vcpkg offers OpenNI2, which we currently don't install on the CI. Maybe we should do that? (independent from this error in TrajkovicKeypoint2D) @larshg

@mvieth mvieth added module: keypoints changelog: fix Meta-information for changelog generation labels Oct 6, 2022
@mvieth mvieth merged commit 753a46e into PointCloudLibrary:master Oct 6, 2022
@SunBlack SunBlack deleted the fix_openmp_msvc branch October 6, 2022 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Meta-information for changelog generation module: keypoints

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants