Split point_types.hpp part into field_traits.(h|hpp)

[sumir0]

• Move (H|h)as_ traits from point_types.hpp into field_traits.(h|hpp)
• Add field_traits.h include where field traits are used
• Change a MSVC compiler warning reset via #pragma warning((push|pop))
• Add field_traits.h include in point_types.h for backward compatibility
• Remove unnecessary pcl/point_types.h includes in some files
• Add missing pcl/point_types.h includes in some files
• Add missing cstring include in pcl/common/impl/copy_point.hpp

[sumir0]

Also may fix #5040

[sumir0]

Based on #6367

[sumir0]

I will try to build manually with PCL_OPTIMIZE_IMPORTS_FIELD_TRAITS. Maybe additional pipelines can be considered.

---

Edit: Compilation finished successfully.

[sumir0]

For a moment I thought I was going under some water here... Phew, it was close! Excuse me for informality. As the pipelines are passing it seems that we are good to go (or maybe not, because there are still some files for which it would be nice to add missing includes, but they can be added in other PRs, possibly)

[larshg]

I tried it on windows yesterday and noticed a lot of: no suitable definition provided for explicit template instantiation request.

See windows ci: https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=25918&view=logs&j=b7dd6ba8-1ad9-5018-2280-3f36c72607c9&t=2fd7de77-c8c6-5479-ec09-1c18859bcc95&l=61

I haven't quite figured out why its not provided, but we should look into it before merging.

[sumir0]

Oh, nice catch! @larshg

As far as I understand these warnings were disabled by pcl/common/include/pcl/pcl_macros.h (link to the latest commit in master at the moment - search for "4661").

pcl/common/include/pcl/pcl_macros.h is included by pcl/common/include/pcl/impl/point_types.hpp which is included by pcl/common/include/pcl/point_types.h.

But pcl/common/include/pcl/pcl_macros.h is not included by pcl/common/include/pcl/field_traits.h.

Therefore, in the translation units where pcl/common/include/pcl/pcl_macros.h is not included (for example: where I replaced pcl/common/include/pcl/point_types.h with pcl/common/include/pcl/field_traits.h and pcl/common/include/pcl/pcl_macros.h did not get to be included) these warnings are not disabled.

[larshg]

Ahh, yes. And it doesn't seem to be worth looking into fixing that warning, so we just need to include pcl_macros.h accordingly?

[sumir0]

Hmm. Disabling warnings in a header file (without returning to the state before the disabling) will affect warnings in user code which includes (directly or transitively) that header. Would it not be better to disable project scope warnings using a build system (in our case CMake)?

[mvieth]

Hmm. Disabling warnings in a header file (without returning to the state before the disabling) will affect warnings in user code which includes (directly or transitively) that header. Would it not be better to disable project scope warnings using a build system (in our case CMake)?

I suppose we could try disabling that warning here: https://github.com/PointCloudLibrary/pcl/blob/master/CMakeLists.txt#L190
But then, would that warning be shown if a user compiles a project that includes certain PCL headers?

[sumir0]

I suppose we could try disabling that warning here: https://github.com/PointCloudLibrary/pcl/blob/master/CMakeLists.txt#L190

Looks like a good place, if you ask me! Though, there is still a room for improvement. For example, add_compile_options and target_compile_options can be considered instead, they will ensure that changes we are doing to CMAKE_CXX_FLAGS will not propagate into higher-level CMake projects (that maybe a case when somebody builds pcl from source as a dependency of another project via include), add_compile_options being more safe-ish, I think. We can examine alternatives later, if necessary.

But then, would that warning be shown if a user compiles a project that includes certain PCL headers?

I assume pcl usually is used with CMake in user projects. In that case CMake has the SYSTEM feature. Please, see the playground I prepared earlier. Basically, it should be possible for us to configure CMake files not to generate warnings in user builds. You might want to see section "Preventing Warnings in Header Files" in the blog I mentioned in the playground's readme.md. Let me know if a more detailed description/explanation is needed.

[mvieth]

You are right, there is definitely CMake code within PCL that could be modernized or improved. @larshg has already improved a lot of CMake code, and is still working on that topic, I believe. However, that is often a bit risky because after a change it can be difficult to verify that everything still works as expected (on all OS, compilers, user projects, etc). Anyway, in this pull request, I would prefer to minimize changes to the CMake code to what is strictly necessary, and perhaps discuss switching to add_compile_options or similar in another issue/pull request.

I also did a test: I changed pcl_macros.h so warning 4661 is not disabled, and when building a project that uses PCL, I did not see any warning of that kind coming from PCL headers. So that might not be a problem after all. If we find out later that it is a problem, we might look here and consider replacing INTERFACE_INCLUDE_DIRECTORIES with INTERFACE_SYSTEM_INCLUDE_DIRECTORIES (following your suggestion of the SYSTEM feature).

So, my suggestions to get this pull request ready for merging soon:

• change pcl_macros.h so warning 4661 is not disabled
• disable the warning in CMakeLists.txt instead by adding /wd4661 to CMAKE_CXX_FLAGS
• then we check again that the warning is not displayed while building PCL, and also not while building another project that uses PCL

Sounds good?