[GitHub] [arrow-adbc] kou commented on a diff in pull request #844: build(c): force C++11 for drivers for R's sake

["kou (via GitHub)" <gi...@apache.org>]

kou commented on code in PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#discussion_r1240405645


##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -60,16 +60,40 @@ if(CXX_LINKER_SUPPORTS_VERSION_SCRIPT)
 endif()
 
 # Set common build options
-macro(adbc_configure_target TARGET)
-  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    target_compile_options(${TARGET}
-                           PRIVATE -Wall
-                                   -Werror
-                                   -Wextra
-                                   -Wpedantic
-                                   -Wno-unused-parameter
-                                   -Wunused-result)
+if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "")

Review Comment:
   Can we simplify this? 
   
   ```suggestion
   if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "")
   ```



##########
c/cmake_modules/BuildUtils.cmake:
##########
@@ -194,6 +194,10 @@ function(ADD_ARROW_LIB LIB_NAME)
     target_link_libraries(${LIB_NAME}_objlib
                           PRIVATE ${ARG_SHARED_LINK_LIBS} ${ARG_SHARED_PRIVATE_LINK_LIBS}
                                   ${ARG_STATIC_LINK_LIBS})
+    adbc_configure_target(${LIB_NAME}_objlib)
+    # https://github.com/apache/arrow-adbc/issues/81
+    target_compile_features(${LIB_NAME}_objlib PRIVATE cxx_std_11)
+    set_property(TARGET ${LIB_NAME}_objlib PROPERTY CXX_STANDARD 11)

Review Comment:
   This may be redundant. (I think that we only need `target_compile_features()`.)



##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -60,16 +60,40 @@ if(CXX_LINKER_SUPPORTS_VERSION_SCRIPT)
 endif()
 
 # Set common build options
-macro(adbc_configure_target TARGET)
-  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    target_compile_options(${TARGET}
-                           PRIVATE -Wall
-                                   -Werror
-                                   -Wextra
-                                   -Wpedantic
-                                   -Wno-unused-parameter
-                                   -Wunused-result)
+if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "")
+  if("${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE")
+    set(ADBC_BUILD_WARNING_LEVEL "PRODUCTION")
+  else()
+    set(ADBC_BUILD_WARNING_LEVEL "CHECKIN")
+  endif()
+endif()
+
+set(ADBC_C_CXX_FLAGS)
+if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
+  if(MSVC)
+    set(ADBC_C_CXX_FLAGS /Wall /WX)
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    set(ADBC_C_CXX_FLAGS -Wall -Werror)
+  else()
+    message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}")
   endif()
+elseif("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "PRODUCTION")
+  if(MSVC)
+    set(CMAKE_C_FLAGS /Wall)

Review Comment:
   Do we need this?



##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -60,16 +60,40 @@ if(CXX_LINKER_SUPPORTS_VERSION_SCRIPT)
 endif()
 
 # Set common build options
-macro(adbc_configure_target TARGET)
-  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    target_compile_options(${TARGET}
-                           PRIVATE -Wall
-                                   -Werror
-                                   -Wextra
-                                   -Wpedantic
-                                   -Wno-unused-parameter
-                                   -Wunused-result)
+if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "")
+  if("${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE")
+    set(ADBC_BUILD_WARNING_LEVEL "PRODUCTION")
+  else()
+    set(ADBC_BUILD_WARNING_LEVEL "CHECKIN")
+  endif()
+endif()
+
+set(ADBC_C_CXX_FLAGS)
+if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
+  if(MSVC)
+    set(ADBC_C_CXX_FLAGS /Wall /WX)
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    set(ADBC_C_CXX_FLAGS -Wall -Werror)
+  else()
+    message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}")
   endif()
+elseif("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "PRODUCTION")
+  if(MSVC)
+    set(CMAKE_C_FLAGS /Wall)
+    set(ADBC_C_CXX_FLAGS /Wall)
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
+         OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    set(ADBC_C_CXX_FLAGS -Wall)
+  else()
+    message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}")
+  endif()
+endif()

Review Comment:
   We may want to use the following style:
   
   ```cmake
   if(MSVC)
     set(ADBC_C_CXX_FLAGS_CHECKIN "/Wall /WX")
     set(ADBC_C_CXX_FLAGS_PRODUCTION "/Wall /WX")
   elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" ...)
     set(ADBC_C_CXX_FLAGS_CHECKIN "...")
     set(ADBC_C_CXX_FLAGS_PRODUCTION "...")
   else()
     ...
   endif()
   
   target_compile_options(${TARGET} PRIVATE ${ADBC_C_CXX_FLAGS_${ADBC_BUILD_WARNING_LEVEL}})
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org