Pcl: Removal of PCL_DEFINITIONS

[SergioRAgostinho]

Just a reminder on the discussion which started at #1478.

The main point, by @Algomorph

It is unclear to me why compile definitions are necessary at all in this case. The file pcl_config.h.in is configured at the time of CMake generation for pcl, and it probably should have all the flags set correctly at that time instead of relying on -DDISABLE_ flags and such. Files that rely on these definitions should include the pcl_config.h header. This way, users don't need to use PCL_DEFINTIONS explicitly at all.

[stefanbuettner]

Talking about PCL_DEFINITIONS, there is this issue #1725

When removing PCL_DEFINITIONS, what about instruction set extensions and client code? Is it sufficient to write the compiler generated defines, such as #define AVX into pcl_config.h such that client code will be compiled appropriately? And if so, does it work for all supported compilers?

[efernandez]

If you remove the PCL_DEFINITIONS then client code has no way to find out the flags (like vectorization extensions) that PCL was compiled with. This information is needed in the CMakeLists.txt file, so those flags are passed to the compiler.

I'm not expert on this, but I've recently run into issues using other software. The client code crashes because of unaligned memory using vectorized operations, unless it's built with the same flags as the library code.

[taketwo]

You are right. In fact, we are working on this in #2100. I think this issue can be closed.

[Algomorph]

The client code has to include pcl_config.h, wherein it will find the preprocessor defines. Only the flags that _aren't_ preprocessor defines should be added via CMake, using the target_compile_definitions command, and only if the client code must be required to use the same flags in order to run (I'm no expert on which ones those are), should should those be added using the PUBLIC argument.

[SergioRAgostinho]

[...] target_compile_options [...]

Check the differences between the options vs the definitions here.

[SergioRAgostinho]

Only half of the issue is being addressed in #2100, which is passing the compiler options (and definitions) to downstream targets. We still need to ensure all preprocessed definitions are strictly in pcl_config.

[kunaltyagi]

Reviving old issue for discussion

PCL_DEFINITIONS are only used in PCLConfig.cmake.in and doc/tutorials/content.

[stale[bot]]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[kunaltyagi]

This is consumer facing code and is potentially creating a breaking change. What should be done here? Keep or remove?

[stale[bot]]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.