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
• Add missing cstring include in pcl/common/impl/copy_point.hpp
• Add 4244, 4267, 4661 MSVC warnings configuration in CMake
• Remove 4244, 4267, 4661 MSVC warnings configuration from pcl_macros.h
• Remove (now) unused type_traits include from point_types.hpp
• Use Eigen::Map in boundary.h and rift.hpp
• Add missing pcl/point_types.h includes in some files
• Remove unnecessary pcl/point_types.h includes in some files

[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?

[sumir0]

No worries! I am genuinely grateful that PCL exists in the state it is now. ...Thanks to the maintainers, contributors, and others (whose work many of us might not really see).

And well done with the test, @mvieth! I am planning on working on the changes you suggested at the end of this week. But let me know if you need the changes sooner, I will see what I can do.

[mvieth]

I am planning on working on the changes you suggested at the end of this week. But let me know if you need the changes sooner, I will see what I can do.

That is perfectly fine, we are not in a hurry 😄

[sumir0]

then we check again that the warning is not displayed while building PCL, and also not while building another project that uses PCL

I don't see 4661 warnings in PCL builds with MSVC (though, there some other warnings), but I didn't build another project which uses PCL with MSVC, I don't think I can do that at the moment. It would be wonderful if someone could check warnings in that case (no need to rush from my point of view).

[mvieth]

I don't see 4661 warnings in PCL builds with MSVC (though, there some other warnings

I still see 4267 and 4244 warnings, for these we should probably do the same thing (not disable in pcl_macros.h, instead disable in CMakeLists.txt).

[sumir0]

I also did see other warnings in the pipelines of 61492b6; please, note:

• 4804 and 4805 warnings in both MSVC builds
• 4293 warnings in x86 MSVC build

I can see these warnings in a recent build in another pull request, therefore I assume we can leave them as they were in this pull request

[mvieth]

Looks good to me, thank you!

[larshg]

Should we add a note when this should be removed and be default?

[mvieth]

I am not sure we can remove the include in the future? I certainly would not remove it before having a compiler warning (like a deprecation warning) for several PCL versions. The warning would have to be displayed if code uses something from pcl/field_traits.h but does not include pcl/field_traits.h directly, only via pcl/point_types.h. No warning should be displayed if nothing from pcl/field_traits.h is used, or if pcl/field_traits.h is included directly. I think that is very difficult to implement, so I am fine with keeping the pcl/field_traits.h include here forever (unless we find a solution for a compiler warning in the future).

[larshg]

I guess its not that easy no.
Could we add a general deprecation warning/info that says some functions/defines will move to field_traits and make it possible to silence it with a #define in user code?

Else this optimization would hardly be used by anyone?

How do you "enable" it now - set a #define before including the header?

[mvieth]

You mean adding a preprocessor warning just before #include <pcl/field_traits.h>, telling users they can silence the warning by adding #define PCL_OPTIMIZE_IMPORTS_FIELD_TRAITS at the top of their code? Might be an option, but feels too invasive in my opinion because everyone would see that warning, not only people who use something from field_traits.h. If we just want people to be aware that this optimization exists, we can advertise PCL_OPTIMIZE_IMPORTS_FIELD_TRAITS e.g. in the release notes. But if we plan to remove #include <pcl/field_traits.h> from point_types.h in the long term, I would rather try to find a way to show a compiler warning only to those who use something from field_traits.h.

[mvieth]

Okay, here is another idea:

1. define some special preprocessor symbol in point_types.h, just before including field_traits.h, let's say PCL_POINT_TYPES_INCLUDES_FIELD_TRAITS
2. Add something like this in field_traits.h:

#ifdef PCL_POINT_TYPES_INCLUDES_FIELD_TRAITS
#define PCL_MAYBE_DEPRECATED [[deprecated("Include <pcl/field_traits.h> before including <pcl/point_types.h>")]] // Alternatively PCL_DEPRECATED(...) macro here?
#else
#define PCL_MAYBE_DEPRECATED
#endif

3. Mark all structs etc. is field_traits.h and field_traits.hpp with PCL_MAYBE_DEPRECATED. The message should make it clear that the struct/whatever is actually not deprecated, but that the user must include field_traits.h directly.

This should work right? We have done something roughly similar here: #5796

[sumir0]

I think we can do that. But the constraint that pcl/field_traits.h has to be included before pcl/point_types.h bothers me a little bit, to be honest. And I don't have a solution for it. Maybe we can do the deprecation in another PR later?

[mvieth]

I think we can do that. But the constraint that pcl/field_traits.h has to be included before pcl/point_types.h bothers me a little bit, to be honest. And I don't have a solution for it.

True, that is not optimal. At least it would be in alphabetical order 😄 Also, users could #define PCL_OPTIMIZE_IMPORTS_FIELD_TRAITS at the top of their code and then be free to include field_traits.h and point_types.h in any order they like.

Maybe we can do the deprecation in another PR later?

I would be fine with that.

[sumir0]

I would be fine with that.

Thank you, I am simply hoping that some idea will strike me (or maybe you). But I don't want to keep you waiting, feel free to do it by yourself or to ask me to do it.

After all the alphabetical requirement is not intended to be permanent