std::size_t modifications for module common

[kunaltyagi]

• Replaced size_t with std::size_t
• Used modern for loops
• Used std::size_t for indices (in function internals) which don't affect class/struct layout
• Some changes to API to replace int with std::size_t for dimensions and indices
• Used auto to hide unimportant types

The aim is to to use std::size_t instead of int in all (most) modules and then convert the indices to PCL_INDEX_TYPE which can be set as uint16_t or std::size_t or even int. This is first in a series of such commits

[taketwo]

Hey, thanks for taking the initiative. The bullet-list summary is quite helpful in understanding the contents of this PR. However, it would be even greater if each of these bullet points was a separate commit. Such an approach makes the review process more manageable. For example, the first change is trivial, so it's enough to quickly scroll through such commit. Item three, however, potentially affects class/struct layout, and requires a careful review.

I consider this a good style to add outstanding TODO items to the PR descriptions. However, you should clearly state whether it's a TODO for future PRs, or you plan to update the present one. (In the latter case you can also mark it as "draft".)

[kunaltyagi]

Item three, however, potentially affects class/struct layout, and requires a careful review.

As I mentioned, no struct level changes. I've kept the changes to inside functions

if each of these bullet points was a separate commit

Sure. I can manage that.

you plan to update the present one

Hopefully the present one. One PR per module, starting with 'common', then 'features' and 'filters'. Then I dunno. Stress testing is the only way I have apart from reading every line of code 😅 so some things get past my grep and segfaults

[taketwo]

Item three, however, potentially affects class/struct layout, and requires a careful review.

As I mentioned, no struct level changes. I've kept the changes to inside functions

One of the purposes of the review is to verify your claim 😉

Hopefully the present one.

👍

[kunaltyagi]

The issue in filters seems to be copyPointCloud<PointT, PointT>(*cloud_in, cloud_out). Can I just change it to cloud_out = *cloud_in; and everything would be copied, right? Does the copy function doesn't have any different functionality apart from a normal copy operation? (Though changing to copyPointCloud(*cloud_in, cloud_out) works too)

EDIT:
Can't compile mls locally. But looks like everything else works apart from pcl_surface

[taketwo]

Does the copy function doesn't have any different functionality apart from a normal copy operation?

In the case of matching point types copyPointCloud() is just a memcpy of point data. (Plus a copy of meta-data, of course).

[taketwo]

Thanks for splitting up the commits. For now, I stopped after reviewing the first two. Let's handle them first and then move on.

[kunaltyagi]

I accidentally rebased one commit (with const auto&). When you're done with the first 2 commits, please ping me, so I can push the other changes regarding renaming the template

I'm posting the diff manually for the accidentally rebased commit (apologies in advance)

diff --git a/common/include/pcl/common/impl/centroid.hpp b/common/include/pcl/common/impl/centroid.hpp
index 6bf892550..1e5a76e32 100644
--- a/common/include/pcl/common/impl/centroid.hpp
+++ b/common/include/pcl/common/impl/centroid.hpp
@@ -88,7 +88,7 @@ pcl::compute3DCentroid (const pcl::PointCloud<PointT> &cloud,
   // If the data is dense, we don't need to check for NaN
   if (cloud.is_dense)
   {
-    for (const auto point: cloud)
+    for (const auto& point: cloud)
     {
       centroid[0] += point.x;
       centroid[1] += point.y;
@@ -101,7 +101,7 @@ pcl::compute3DCentroid (const pcl::PointCloud<PointT> &cloud,
   }
   // NaN or Inf values could exist => check for them
   unsigned cp = 0;
-  for (const auto point: cloud)
+  for (const auto& point: cloud)
   {
     // Check if the point is invalid
     if (!isFinite (point))
@@ -132,7 +132,7 @@ pcl::compute3DCentroid (const pcl::PointCloud<PointT> &cloud,
   // If the data is dense, we don't need to check for NaN
   if (cloud.is_dense)
   {
-    for (const int &index : indices)
+    for (const int& index : indices)
     {
       centroid[0] += cloud[index].x;
       centroid[1] += cloud[index].y;
@@ -144,7 +144,7 @@ pcl::compute3DCentroid (const pcl::PointCloud<PointT> &cloud,
   }
   // NaN or Inf values could exist => check for them
     unsigned cp = 0;
-  for (const int &index : indices)
+  for (const int& index : indices)
   {
     // Check if the point is invalid
     if (!isFinite (cloud [index]))
diff --git a/common/include/pcl/common/impl/io.hpp b/common/include/pcl/common/impl/io.hpp
index fe170ae68..9de4b9534 100644
--- a/common/include/pcl/common/impl/io.hpp
+++ b/common/include/pcl/common/impl/io.hpp
@@ -64,7 +64,7 @@ pcl::getFieldIndex (const std::string &field_name,
   // Get the fields list
   pcl::for_each_type<typename pcl::traits::fieldList<PointT>::type>(pcl::detail::FieldAdder<PointT>(fields));
   const auto result = std::find_if(fields.begin (), fields.end (),
-      [&field_name](const auto field) { return field.name == field_name; });
+      [&field_name](const auto& field) { return field.name == field_name; });
   if (result == fields.end ())
     return -1;
   return std::distance(fields.begin (), result);
diff --git a/common/include/pcl/conversions.h b/common/include/pcl/conversions.h
index 9a4c40e3e..62f9eb6ff 100644
--- a/common/include/pcl/conversions.h
+++ b/common/include/pcl/conversions.h
@@ -314,7 +314,7 @@ namespace pcl
   inline void
   toPCLPointCloud2 (const pcl::PCLPointCloud2& cloud, pcl::PCLImage& msg)
   {
-    const auto predicate = [](const auto field) { return field.name == "rgb"; };
+    const auto predicate = [](const auto& field) { return field.name == "rgb"; };
     const auto result = std::find_if(cloud.fields.cbegin (), cloud.fields.cend (), predicate);
     if (result == cloud.fields.end ())
       throw std::runtime_error ("No rgb field!!");
diff --git a/common/src/PCLPointCloud2.cpp b/common/src/PCLPointCloud2.cpp
index d3537a15d..85bdaa1af 100644
--- a/common/src/PCLPointCloud2.cpp
+++ b/common/src/PCLPointCloud2.cpp
@@ -152,7 +152,7 @@ pcl::PCLPointCloud2::concatenate (pcl::PCLPointCloud2 &cloud1, const pcl::PCLPoi
   }
   for (std::size_t cp = 0; cp < size2; ++cp)
   {
-    for (const auto field_data: valid_fields)
+    for (const auto& field_data: valid_fields)
     {
       const auto& i = field_data.idx1;
       const auto& j = field_data.idx2;

[taketwo]

No worries. Yes, I think I'm done with the first two.

[kunaltyagi]

Oops, pressed the wrong button. Vim plugins for browsers should have an undo 😢

[SergioRAgostinho]

• Replaced size_t with std::size_t

What's the rationale on this?

[kunaltyagi]

What's the rationale on this?

My understanding of the standard says that the header <cstddef> may or may not declare ::size_t, so it cannot be relied upon for making sure that ::size_t is present or absent, unless specifically including <stddef.h> or another header from the C library which is guaranteed to declare it.

Since we are slowly migrating away from C functions, there's no guarantee that the all stdlib will provide the optional declaration of ::size_t. As such it doesn't make sense to keep using size_t when we actually mean std::size_t.

We can declare in pcl namespace using std::size_t as an alternative

[taketwo]

TL;DR: same reason as migrating from abs to std::abs. Former is a legacy C code, and the latter is proper C++.

[kunaltyagi]

I'll check this code for use of reserve before proceeding

[kunaltyagi]

No changes, just a rebase to bring everything up to date (MacOS CI, conflict due to vector.reserve())

[SergioRAgostinho]

I accepted the changes, however now one of the tutorials in failing to build. I'm unsure if is related to this PR though.

[taketwo]

Yep, vhf_recognition tutorial is using the now-deprecated function, and warnings are treated as errors.

I think it should be a rule that when we deprecate something, we grep through the codebase and remove usage of deprecated functions from PCL itself. (This, by the way, has not been done for the previous deprecation, see #3349.)

[taketwo]

LGTM, please squash as appropriate.

Edit: and resolve conflicts

[kunaltyagi]

PTAL

[taketwo]

Thanks. The new separation into commits makes sense to me, except for one case. In the "Letting the compiler auto-deduce template parameters" commit the changes to io.h and io.hpp do not belong there I think. These changes are updating the lines touched by "Better loops via modern for, algorithms or redirection" and thus should be squashed with it.

[kunaltyagi]

Another flaky test found:

[----------] 1 test from SacTest/1, where TypeParam = pcl::LeastMedianSquares<pcl::PointXYZ>
111: [ RUN      ] SacTest/1.InfiniteLoop
111: /__w/1/s/test/sample_consensus/test_sample_consensus.cpp:134: Failure
111: Value of: cv.wait_for (lock, 1s)
111:   Actual: 4-byte object <01-00 00-00>
111: Expected: std::cv_status::no_timeout
111: Which is: 4-byte object <00-00 00-00>
111: [  FAILED  ] SacTest/1.InfiniteLoop, where TypeParam = pcl::LeastMedianSquares<pcl::PointXYZ> (1001 ms)
111: [----------] 1 test from SacTest/1 (1001 ms total)

[kunaltyagi]

Fixed the separation issue after a mistake 😭 . PTAL