PingLiuPing commented on code in PR #936: URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4033502467
########## example/CMakeLists.txt: ########## @@ -20,13 +20,65 @@ cmake_minimum_required(VERSION 3.25) project(example) -set(CMAKE_CXX_STANDARD 23) Review Comment: @zhjwpku Thank you for the comments. Making it configurable is a good idea. Have different thoughts on making C++23 as default though. With this patch, it changes the minimum supported C++ standard from C++23 to C++20 for downstream consumers. And C++20 is the interface contract between the consumer and iceberg-cpp library, and the contract should be tested continuously. Setting the default to C++20 ensures the minimum supported standard (contract) is continuously exercised. Defaulting it to C++23 would let C++20 only breakages slip through. One refinement is that C++23 compatibility should still be tested separately. C++23 should accepts C++20 code, but we can enhance this by provide an optional example configuration for C++23, for example, expose an `ICEBERG_EXAMPLE_CXX_STANDARD` cache setting that defaults to 20 and accepts 23; then update CI to build both. And also refine the document to state clearly that the minimum C++ standard is C++20 for public headers. What do you think? Happy to make changes either way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
