Copilot commented on code in PR #874:
URL: https://github.com/apache/incubator-graphar/pull/874#discussion_r2847629455
##########
cpp/CMakeLists.txt:
##########
@@ -460,9 +467,17 @@ else()
message(FATAL_ERROR "parquet is required, please install it and retry")
endif()
+ # Update C++ standard based on detected system Arrow version
+ if (Arrow_VERSION VERSION_GREATER_EQUAL "23.0.0")
Review Comment:
Inconsistent variable reference style: line 471 uses `Arrow_VERSION` without
braces while line 451 in the same file uses `${Arrow_VERSION}`. For
consistency, consider using `${Arrow_VERSION}` or wrapping it in quotes. While
both forms work in if() statements in CMake, consistent style improves
readability.
```suggestion
if (${Arrow_VERSION} VERSION_GREATER_EQUAL "23.0.0")
```
##########
cpp/CMakeLists.txt:
##########
@@ -426,7 +427,13 @@ if (BUILD_ARROW_FROM_SOURCE)
endif()
endif()
find_package(CURL REQUIRED)
-
+ if(DEFINED ENV{GRAPHAR_ARROW_VERSION})
+ if ($ENV{GRAPHAR_ARROW_VERSION} VERSION_GREATER_EQUAL "23.0.0")
+ set(GAR_CXX_STANDARD 20)
+ endif()
+ else()
+ set(GAR_CXX_STANDARD 20) # Default Arrow version is 23.0.0 need C++20
+ endif()
include(apache-arrow)
Review Comment:
The logic assumes that if GRAPHAR_ARROW_VERSION is not set, Arrow 23.0.0
will be built (requiring C++20). However, this assumption is implicit and
depends on the default in apache-arrow.cmake. If someone modifies the default
Arrow version in apache-arrow.cmake to be less than 23.0.0, this logic would
incorrectly set C++20. Consider reading the ARROW_VERSION_TO_BUILD variable
that's set in apache-arrow.cmake instead of duplicating the version check logic.
```suggestion
# Include apache-arrow configuration to determine ARROW_VERSION_TO_BUILD
include(apache-arrow)
# Update C++ standard based on Arrow version to build
if (ARROW_VERSION_TO_BUILD VERSION_GREATER_EQUAL "23.0.0")
set(GAR_CXX_STANDARD 20)
endif()
```
##########
cpp/CMakeLists.txt:
##########
@@ -147,7 +149,6 @@ set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake)
include_directories(${CMAKE_CURRENT_SOURCE_DIR})
add_library(${PROJECT_NAME} INTERFACE)
-target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_17)
target_include_directories(
${PROJECT_NAME}
INTERFACE
Review Comment:
The removal of `target_compile_features(${PROJECT_NAME} INTERFACE
cxx_std_17)` from the interface library may cause issues. This interface
library is likely used by consumers of the GraphAr library, and removing the
C++ standard requirement means dependent projects will not automatically
inherit the C++ standard requirement. Consider setting
`target_compile_features(${PROJECT_NAME} INTERFACE
cxx_std_${GAR_CXX_STANDARD})` instead of removing it entirely, or document that
consumers must set their own C++ standard.
##########
cpp/CMakeLists.txt:
##########
@@ -426,7 +427,13 @@ if (BUILD_ARROW_FROM_SOURCE)
endif()
endif()
find_package(CURL REQUIRED)
-
+ if(DEFINED ENV{GRAPHAR_ARROW_VERSION})
+ if ($ENV{GRAPHAR_ARROW_VERSION} VERSION_GREATER_EQUAL "23.0.0")
+ set(GAR_CXX_STANDARD 20)
+ endif()
+ else()
+ set(GAR_CXX_STANDARD 20) # Default Arrow version is 23.0.0 need C++20
Review Comment:
The comment has a grammatical issue: "Default Arrow version is 23.0.0 need
C++20" should be "Default Arrow version is 23.0.0 needs C++20" or "Default
Arrow version 23.0.0 needs C++20".
```suggestion
set(GAR_CXX_STANDARD 20) # Default Arrow version 23.0.0 needs C++20
```
##########
cpp/CMakeLists.txt:
##########
@@ -104,11 +104,13 @@ if(NOT (CMAKE_CXX_COMPILER_LAUNCHER MATCHES "ccache") AND
NOT (CMAKE_C_COMPILER_
endif(ccache_EXECUTABLE)
endif()
+set(GAR_CXX_STANDARD 17)
+
if(MSVC)
# Avoid GCC/Clang-specific flags on MSVC.
# C++17 is already enforced via CMAKE_CXX_STANDARD/target features.
Review Comment:
The comment on line 111 states "C++17 is already enforced via
CMAKE_CXX_STANDARD/target features" for MSVC, but there is no
CMAKE_CXX_STANDARD set anywhere in the CMakeLists.txt file. With the removal of
`-std=c++17` from CMAKE_CXX_FLAGS and the interface library's
target_compile_features, MSVC builds now rely solely on target_compile_features
for individual targets. This is fine for the graphar library, tests, and
benchmarks which have target_compile_features set, but examples do not have
this set and may fail to compile on MSVC with C++20 requirements.
--
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]
