wgtmac commented on code in PR #936: URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4090576050
########## mkdocs/docs/getting-started.md: ########## @@ -23,10 +23,14 @@ **Required:** -- C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+) +- C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+) to build iceberg-cpp itself Review Comment: It seems better to keep here unchanged to indicate that we officially support C++23. Then we can add a dedicated section below for the contract of C++20 compatibility. ########## src/iceberg/expected.h: ########## @@ -0,0 +1,2443 @@ +/* + * MIT License + * + * Copyright (c) 2024 zeus-cpp + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/// \file iceberg/expected.h +/// \brief A C++20 backport of C++23 `std::expected`, used as the storage type +/// behind `iceberg::Result`. +/// +/// Purpose: the library itself may be built as C++23, but its public headers +/// must stay consumable from C++20 translation units. `std::expected` is a +/// C++23 library feature, so `iceberg::Result<T>` (see iceberg/result.h) is +/// defined in terms of `iceberg::expected` instead. The API mirrors +/// `std::expected` (including the monadic `and_then`, `or_else`, `transform` +/// and `transform_error`), so code can be written as if against the standard +/// type. +/// +/// History: +/// - apache/iceberg-cpp#40 vendored this header, adapted from +/// https://github.com/zeus-cpp/expected (MIT), while the project targeted Review Comment: Could we pin the vendored source to a specific upstream tag/commit and record the local delta here? The current history only links the repository, so future audits and upstream syncs will not know which version this 2.4k-line file came from. ########## example/CMakeLists.txt: ########## @@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25) project(example) -set(CMAKE_CXX_STANDARD 23) +# C++20 is the minimum standard iceberg-cpp's public headers support, so the +# example builds as C++20 by default to keep that contract exercised. Set this to +# 23 to also check the headers from a C++23 consumer. +set(ICEBERG_EXAMPLE_CXX_STANDARD + 20 + CACHE STRING "C++ standard used to build the example (20 or 23)") +set_property(CACHE ICEBERG_EXAMPLE_CXX_STANDARD PROPERTY STRINGS 20 23) +if(NOT ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(20|23)$") + message(FATAL_ERROR "ICEBERG_EXAMPLE_CXX_STANDARD must be 20 or 23, got " + "'${ICEBERG_EXAMPLE_CXX_STANDARD}'") +endif() + +set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD}) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_CXX_EXTENSIONS OFF) find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest) +if(TARGET iceberg::iceberg_bundle_shared) + set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared) +else() + set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static) +endif() + +if(TARGET iceberg::iceberg_rest_shared) + set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared) +else() + set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static) +endif() + add_executable(demo_example demo_example.cc) -target_link_libraries(demo_example - PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>" - "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>" -) +target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET} + ${ICEBERG_REST_TARGET}) + +# Compile every installed public header as a consumer using +# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include +# tree is the public API contract: iceberg_install_all_headers excludes internal +# headers before packaging them. +get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET} + INTERFACE_INCLUDE_DIRECTORIES) +foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS) + if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg") + set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}") + break() + endif() +endforeach() + +if(NOT ICEBERG_PUBLIC_INCLUDE_DIR) + message(FATAL_ERROR "Could not locate iceberg's installed public headers") +endif() + +file(GLOB_RECURSE + ICEBERG_PUBLIC_HEADERS + CONFIGURE_DEPENDS + "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h" + "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp") Review Comment: nit: we don't have `.hpp`, do we? ########## example/CMakeLists.txt: ########## @@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25) project(example) -set(CMAKE_CXX_STANDARD 23) +# C++20 is the minimum standard iceberg-cpp's public headers support, so the +# example builds as C++20 by default to keep that contract exercised. Set this to +# 23 to also check the headers from a C++23 consumer. +set(ICEBERG_EXAMPLE_CXX_STANDARD + 20 + CACHE STRING "C++ standard used to build the example (20 or 23)") +set_property(CACHE ICEBERG_EXAMPLE_CXX_STANDARD PROPERTY STRINGS 20 23) +if(NOT ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(20|23)$") + message(FATAL_ERROR "ICEBERG_EXAMPLE_CXX_STANDARD must be 20 or 23, got " Review Comment: Do we really need to specify 20 or 23 here? We need to update this file as well when we support C++26. ########## src/iceberg/util/error_collector.h: ########## @@ -101,6 +104,13 @@ class ICEBERG_EXPORT ErrorCollector { ErrorCollector(const ErrorCollector&) = default; ErrorCollector& operator=(const ErrorCollector&) = default; +// C++23 uses deducing `this` so that `return AddError(...)` keeps returning the Review Comment: Both this class and `SnapshotUpdate` now duplicate the full API and comment blocks across the C++20/C++23 branches. Could we centralize the deducing-`this` feature test and avoid maintaining two implementations, or at least keep one API shape unless derived-type fluent chaining is a required compatibility guarantee? ########## example/CMakeLists.txt: ########## @@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25) project(example) -set(CMAKE_CXX_STANDARD 23) +# C++20 is the minimum standard iceberg-cpp's public headers support, so the +# example builds as C++20 by default to keep that contract exercised. Set this to +# 23 to also check the headers from a C++23 consumer. +set(ICEBERG_EXAMPLE_CXX_STANDARD + 20 + CACHE STRING "C++ standard used to build the example (20 or 23)") +set_property(CACHE ICEBERG_EXAMPLE_CXX_STANDARD PROPERTY STRINGS 20 23) +if(NOT ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(20|23)$") + message(FATAL_ERROR "ICEBERG_EXAMPLE_CXX_STANDARD must be 20 or 23, got " + "'${ICEBERG_EXAMPLE_CXX_STANDARD}'") +endif() + +set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD}) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_CXX_EXTENSIONS OFF) find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest) +if(TARGET iceberg::iceberg_bundle_shared) + set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared) +else() + set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static) +endif() + +if(TARGET iceberg::iceberg_rest_shared) + set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared) +else() + set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static) +endif() + add_executable(demo_example demo_example.cc) -target_link_libraries(demo_example - PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>" - "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>" -) +target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET} + ${ICEBERG_REST_TARGET}) + +# Compile every installed public header as a consumer using +# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include +# tree is the public API contract: iceberg_install_all_headers excludes internal +# headers before packaging them. +get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET} + INTERFACE_INCLUDE_DIRECTORIES) +foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS) + if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg") + set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}") + break() + endif() +endforeach() + +if(NOT ICEBERG_PUBLIC_INCLUDE_DIR) + message(FATAL_ERROR "Could not locate iceberg's installed public headers") +endif() + +file(GLOB_RECURSE + ICEBERG_PUBLIC_HEADERS + CONFIGURE_DEPENDS + "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h" + "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp") +list(SORT ICEBERG_PUBLIC_HEADERS) + +set(ICEBERG_PUBLIC_HEADER_CHECK_SOURCE + "// Generated from iceberg's installed public headers.\n") +foreach(ICEBERG_PUBLIC_HEADER IN LISTS ICEBERG_PUBLIC_HEADERS) + file(RELATIVE_PATH ICEBERG_PUBLIC_HEADER_RELATIVE_PATH "${ICEBERG_PUBLIC_INCLUDE_DIR}" + "${ICEBERG_PUBLIC_HEADER}") + string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE + "#include <${ICEBERG_PUBLIC_HEADER_RELATIVE_PATH}>\n") +endforeach() +string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE "\nint main() { return 0; }\n") + +file(GENERATE + OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc" + CONTENT "${ICEBERG_PUBLIC_HEADER_CHECK_SOURCE}") + +add_executable(public_headers_check "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc") Review Comment: IMO, a better alternative is to add a dedicated test executable built with C++20. It takes extra steps to install iceberg libraries and then build the example. We can add a non-installed header file (e.g. src/iceberg/cpp20_compatibility_internal.h) to include all public headers and then use it in the test case. The challenge is to make this header file in sync when we add new header files. We can update AGENTS.md to add this as an advice. -- 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]
