kou commented on code in PR #12914: URL: https://github.com/apache/arrow/pull/12914#discussion_r899724290
########## cpp/cmake_modules/SetupCxxFlags.cmake: ########## @@ -118,12 +118,16 @@ if(NOT DEFINED CMAKE_C_STANDARD) set(CMAKE_C_STANDARD 11) endif() -# This ensures that things like c++11 get passed correctly +# This ensures that things like c++11/c++14 get passed correctly if(NOT DEFINED CMAKE_CXX_STANDARD) - set(CMAKE_CXX_STANDARD 11) + if(ARROW_AZURE) + set(CMAKE_CXX_STANDARD 14) Review Comment: I think that it's reasonable to use project global `CMAKE_CXX_STANDARD=14` with `-DARROW_AZURE=ON`. It'll reduce our CMake related maintenance cost. Here are reasons: * CMake provides `CXX_STANDARD` https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html target property to control C++ version but it's only for CMake target not for source. It means that we need to create a target for `azurefs.cc` to use `CXX_STANDARD`. In general, we should use object library https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries for this but CMake 3.5 (our minimum required version) doesn't provide enough convenient features for object library. For example, we can't use object library for `target_link_libraries()`. ("New in version 3.12: Object libraries can be linked to with [target_link_libraries()](https://cmake.org/cmake/help/latest/command/target_link_libraries.html#command:target_link_libraries).") I don't want to maintain such CMake code for this case... * Project global `CMAKE_CXX_STANDARD=14` doesn't cause a new problem. Our code base should work with `cmake -DCMAKE_CXX_STANDARD=14 ...` and `cmake -DCMAKE_CXX_STANDARD=17 ...`. For example, we have tests for these cases: https://github.com/apache/arrow/blob/master/dev/tasks/tasks.yml#L1189-L1198 -- 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]
