Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile PCL as C++17 by default, switching back to C++14 currently still possible #6201

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Dec 15, 2024

Mainly due to https://eigen.tuxfamily.org/dox/group__TopicStructHavingEigenMembers.html Before C++17, certain PCL classes/structs have to be annotated with EIGEN_MAKE_ALIGNED_OPERATOR_NEW. Since C++17, that macro is empty, which creates possible incompatibilities between PCL and the user project. #5793 implemented a workaround, which meant that the old mechanism of EIGEN_MAKE_ALIGNED_OPERATOR_NEW was still used in newer C++ standards. Since many compilers support C++17 now, making it the new default in PCL seems logical. According to Wikipedia, these compilers have full support for C++17: https://en.wikipedia.org/wiki/C%2B%2B17#Compiler_support
Additionally, GCC 7, Clang 4, and MSVC >= 19.12 might work since they support the relevant C++17 feature "Dynamic memory allocation for over-aligned data".
Related issue: #6126
With this change, PCL will be compiled as C++17 by default (previously: C++14 by default). Compiling as C++14 is currently still possible via -DCMAKE_CXX_STANDARD=14 -DCMAKE_CUDA_STANDARD=14.
If PCL is compiled as C++17 or newer, projects using PCL have to be compiled as C++17 or newer (this was previously not enforced). When using CMake, this is done automatically.
If PCL is compiled as C++14, projects using PCL have to be compiled as C++14 or newer. However, EIGEN_HAS_CXX17_OVERALIGN=0 has to be defined in the user project (when using CMake, this is done automatically).

Note: MSC_VER 1912 is chosen because since that version, the C++17 feature "Dynamic memory allocation for over-aligned data" is available in MSVC. Eigen uses the same version number to determine EIGEN_HAS_CXX17_OVERALIGN.
The CUDA compiler on Ubuntu 20.04 (CUDA toolkit 10.1) does not work with C++17/CUDA17.

The clang-tidy warnings regarding modernize-use-auto only appeared after setting the compile default to C++17, for some reason.

@mvieth mvieth added changelog: behavior change Meta-information for changelog generation module: cmake labels Dec 15, 2024
@mvieth mvieth changed the title Require at least C++17 Compile PCL as C++17 by default, switching back to C++14 currently still possible Dec 16, 2024
@mvieth mvieth marked this pull request as ready for review December 16, 2024 19:27
@larshg
Copy link
Contributor

larshg commented Dec 20, 2024

But I guess the first introduced c++17 feature, like using std::optional or any other feature, we would not support c++14 anymore?
So one should always have two implementations branches, one that works with c++17 and one for c++14?
Should we rather just make the switch and not necessarily support c++14 from PCL 1.15 and onwards?

@mvieth
Copy link
Member Author

mvieth commented Dec 20, 2024

But I guess the first introduced c++17 feature, like using std::optional or any other feature, we would not support c++14 anymore? So one should always have two implementations branches, one that works with c++17 and one for c++14? Should we rather just make the switch and not necessarily support c++14 from PCL 1.15 and onwards?

Yes, you are right. If we use a C++17 feature, we need to add a C++14 alternative branch to stay compatible with C++14. for std::optional for example, that could be boost::optional. In pcl_macros.h, we also already have some logic for this. It is true that it would be easier to just leave C++14 compatibility behind us.
On the other hand, I can't think of any C++17 feature that we need immediately, where we can't create a good, C++14 compatible workaround. Sure, some things are easier and "cleaner" with C++17 features, but I think the cost for C++14 compatibility is not too high right now. The main reason why I believe PCL 1.15.0 (and possibly 1.15.1) should be C++14 compatible is that the default CUDA compiler (the one from apt) on Ubuntu 20.04 does not work with C++17. The (standard) support for Ubuntu 20.04 ends in May next year, after that I am okay with dropping support for C++14 in PCL. Until then, in my opinion, the solution in this pull request to having to switch back to C++14 if necessary, is a good signal and "soft transition" to C++17.

@mvieth mvieth merged commit 42f3eda into PointCloudLibrary:master Dec 20, 2024
13 checks passed
@mvieth mvieth deleted the require_c++17 branch December 20, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: behavior change Meta-information for changelog generation module: cmake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants