bkietz commented on code in PR #573:
URL: https://github.com/apache/arrow-nanoarrow/pull/573#discussion_r1704571334
##########
CMakeLists.txt:
##########
@@ -145,41 +147,17 @@ target_include_directories(nanoarrow
PUBLIC
$<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>
$<INSTALL_INTERFACE:include>)
-target_compile_definitions(nanoarrow PUBLIC
"$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
-
-# Ensure targets/headers can be installed
-install(TARGETS nanoarrow
- DESTINATION lib
- EXPORT nanoarrow-exports)
-
install(FILES ${NANOARROW_INSTALL_HEADERS} DESTINATION include/nanoarrow)
-if(CMAKE_BUILD_TYPE STREQUAL "Debug")
- if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wno-type-limits
- -Wmaybe-uninitialized
- -Wunused-result
- -Wconversion
- -Wno-sign-conversion)
- elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID
STREQUAL
- "Clang")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wdocumentation
- -Wconversion
- -Wno-sign-conversion)
+if(NANOARROW_IPC)
+ # flatcc requires C11 for alignas() and static_assert() in flatcc_generated.h
+ # It may be possible to use C99 mode to build the runtime and/or generated
header
Review Comment:
If `alignas` and `static_assert` are the only non-c99 features used by
flatcc_generated.h, it should be possible to define
```c
#if __STDC_VERSION__ < 201112L
#define alignas(...)
#define static_assert(...)
#endif
```
( [macro
reference](https://en.cppreference.com/w/c/preprocessor/replace#Predefined_macros)
)
##########
CMakeLists.txt:
##########
@@ -346,9 +339,50 @@ if(NANOARROW_BUILD_TESTS OR
NANOARROW_BUILD_INTEGRATION_TESTS)
PUBLIC
$<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>
$<INSTALL_INTERFACE:include>)
- target_link_libraries(nanoarrow_c_data_integration PRIVATE nanoarrow
nanoarrow_testing)
+ target_link_libraries(nanoarrow_c_data_integration PRIVATE nanoarrow_testing)
endif()
+# Common configuration for all targets
+foreach(target
+ nanoarrow
+ nanoarrow_ipc
+ nanoarrow_device
+ nanoarrow_testing
+ nanoarrow_c_data_integration)
+ if(TARGET ${target})
+ target_compile_definitions(${target} PUBLIC
"$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
+
+ install(TARGETS ${target}
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+
+ if(CMAKE_BUILD_TYPE STREQUAL "Debug")
Review Comment:
It's possibly not a concern for nanoarrow, but CMake doesn't define this
variable in all generators. Specifically it's defined for single configuration
generators like Ninja or makefiles, but on multi configuration generators like
Visual Studio or XCode we have instead
https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html
I think the most correct way to do this is by wrapping these options in a
generator expression:
```
target_compile_options(${target} PRIVATE "$<$<CONFIG:Debug>:-Wall -Werror
-Wextra ...>")
```
--
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]
