paleolimbot commented on code in PR #44:
URL: https://github.com/apache/arrow-nanoarrow/pull/44#discussion_r969052992
##########
src/nanoarrow/schema.c:
##########
@@ -1026,7 +1032,7 @@ ArrowErrorCode ArrowSchemaViewInit(struct
ArrowSchemaView* schema_view,
return EINVAL;
}
- int format_len = strlen(format);
+ unsigned long format_len = strlen(format);
Review Comment:
Maybe `size_t`? I'm sure `unsigned long` is fine, we just don't use it
anywhere else.
##########
src/nanoarrow/schema.c:
##########
@@ -591,7 +591,7 @@ static ArrowErrorCode ArrowSchemaViewParse(struct
ArrowSchemaView* schema_view,
return EINVAL;
}
- schema_view->fixed_size = strtol(format + 2, (char**)format_end_out, 10);
+ schema_view->fixed_size = (int32_t)strtol(format + 2,
(char**)format_end_out, 10);
Review Comment:
I'm not too worried about this, but you could also change `struct
SchemaView::fixed_size` to be an `int64_t`.
##########
CMakeLists.txt:
##########
@@ -89,6 +89,29 @@ else()
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
$<INSTALL_INTERFACE:include>)
target_include_directories(nanoarrow PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated>)
+ if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
Review Comment:
Should this be fenced in a `if(CMAKE_BUILD_TYPE EQUAL "Debug")`? If somebody
is building this as a dependency of a dependency or something it seems a bit
rude to foist warnings on them (or maybe this doesn't do that or maybe that's
common?)
##########
CMakeLists.txt:
##########
@@ -89,6 +89,29 @@ else()
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
$<INSTALL_INTERFACE:include>)
target_include_directories(nanoarrow PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated>)
+ if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
+ target_compile_options(
+ nanoarrow
+ PRIVATE
+ -Wall
+ -Werror
+ -Wextra
+ -Wno-type-limits
+ -Wno-unused-parameter
+ -Wpedantic
+ -Wunused-result)
+ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR
+ CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
Review Comment:
Above this is `CMAKE_C_COMPILER_ID` (as opposed to `CXX`)...does that matter?
--
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]
