Skip to content

Conversation

@jmackay2
Copy link
Contributor

This adds C++20 compiler features. Previously you could select a newer C++ version, but it always set the PCL_CXX_COMPILE_FEATURES to 17.

This also updates the CMAKE_CUDA_STANDARD to use the same standard as the CXX_STANDARD. The CMAKE_CUDA_STANDARD could previously be set to a newer value than CXX_STANDARD, but compilation issues occur from pcl_config.h.in if the CMAKE_CUDA_STANDARD is ever less than the CMAKE_CXX_STANDARD. This change enforces they both use the same standard.

Testing

This was tested by successfully building with the CMAKE_CXX_STANDARD set to both 17 and 20.

@larshg larshg requested a review from Copilot January 14, 2026 06:19
@larshg larshg added module: cmake changelog: enhancement Meta-information for changelog generation labels Jan 14, 2026
@larshg larshg added this to the pcl-1.16.0 milestone Jan 14, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds C++20 compiler support to PCL and synchronizes the CUDA standard with the C++ standard to prevent compilation issues.

Changes:

  • Added C++20 support with appropriate compiler feature flags and version checks
  • Fixed PCL_CXX_COMPILE_FEATURES to dynamically reflect the selected C++ standard instead of being hardcoded to C++17
  • Synchronized CMAKE_CUDA_STANDARD with CMAKE_CXX_STANDARD to prevent mismatches that cause compilation errors

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

set(CMAKE_CXX_EXTENSIONS OFF)
if("${CMAKE_CXX_STANDARD}" GREATER_EQUAL 17)
set(PCL_CXX_COMPILE_FEATURES cxx_std_17)
if("${CMAKE_CXX_STANDARD}" GREATER_EQUAL 20)
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition GREATER_EQUAL 20 will match C++ standards beyond C++20 (such as C++23, C++26, etc.), but sets C++20-specific values for PCL__cplusplus (202002L) and PCL_REQUIRES_MSC_VER (1928). This creates an inconsistency: if a user sets CMAKE_CXX_STANDARD to 23, line 21 would set PCL_CXX_COMPILE_FEATURES to cxx_std_23, but PCL__cplusplus would remain at the C++20 value (202002L). This could cause incorrect compiler version checks in pcl_config.h.in. Consider either changing to EQUAL 20 with additional conditions for newer standards, or update the values to be appropriate for the highest standard being checked.

Suggested change
if("${CMAKE_CXX_STANDARD}" GREATER_EQUAL 20)
if("${CMAKE_CXX_STANDARD}" EQUAL 20)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one we can just leave as is. PCL__cplusplus is only used to check for a minimum requirement.

endif()

set(CMAKE_CUDA_STANDARD 17 CACHE STRING "The target CUDA/C++ standard. PCL requires CUDA/C++ 14 or higher.")
set(PCL_CXX_COMPILE_FEATURES cxx_std_${CMAKE_CXX_STANDARD})
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CMAKE_CUDA_STANDARD is no longer defined as a CACHE variable. While the PR description explains this enforces synchronization with CMAKE_CXX_STANDARD to prevent compilation issues, this is a breaking change for users who were previously setting CMAKE_CUDA_STANDARD separately via CMake command line (e.g., -DCMAKE_CUDA_STANDARD=20). Their setting will now be ignored. Consider documenting this breaking change in release notes or migration guides, or alternatively, implementing a check that warns users if they attempt to set CMAKE_CUDA_STANDARD to a different value than CMAKE_CXX_STANDARD.

Suggested change
set(PCL_CXX_COMPILE_FEATURES cxx_std_${CMAKE_CXX_STANDARD})
set(PCL_CXX_COMPILE_FEATURES cxx_std_${CMAKE_CXX_STANDARD})
# Warn users if they attempted to set CMAKE_CUDA_STANDARD independently; it is
# intentionally synchronized with CMAKE_CXX_STANDARD in this project.
if(DEFINED CMAKE_CUDA_STANDARD AND NOT CMAKE_CUDA_STANDARD STREQUAL "${CMAKE_CXX_STANDARD}")
message(WARNING
"CMAKE_CUDA_STANDARD is synchronized with CMAKE_CXX_STANDARD in this project. "
"The provided CMAKE_CUDA_STANDARD ('${CMAKE_CUDA_STANDARD}') will be ignored in "
"favor of CMAKE_CXX_STANDARD='${CMAKE_CXX_STANDARD}'.")
endif()

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one we could consider adding.

@mvieth
Copy link
Member

mvieth commented Jan 14, 2026

Sorry, I do not yet understand the necessity of this change (apart from CMAKE_CUDA_STANDARD).
If I e.g. specify cmake .. -DCMAKE_CXX_STANDARD=23, PCL is indeed built as C++23 (corresponding flags are passed to the compiler). PCL_CXX_COMPILE_FEATURES, PCL__cplusplus, and PCL_REQUIRES_MSC_VER are mainly relevant for other projects that use PCL, to make sure that the compiler there uses a C++ standard high enough. E.g. PCL uses lots of C++14 features, so a downstream project could not use the C++11 standard. However, PCL does not use any C++20 features currently (as far as I recall, please correct me if I am wrong), so I do not see the point of requiring downstream projects to use at least C++20 if PCL itself was compiled as C++20. With the master branch, it is for example possible to build PCL as C++20 and a downstream project as C++17, which to my knowledge works fine and I see no reason to disallow that. So in short, I do not see which problem you are trying to fix with these changes?

@jmackay2
Copy link
Contributor Author

@mvieth I agree. I was looking at using a few C++20 features, and I was starting with this change. I agree that I don't think this change actually fixes anything.

@jmackay2 jmackay2 closed this Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: enhancement Meta-information for changelog generation module: cmake

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants