paleolimbot commented on code in PR #483:
URL: https://github.com/apache/arrow-nanoarrow/pull/483#discussion_r1634773302
##########
src/apps/dump_stream.c:
##########
@@ -23,8 +23,6 @@
void dump_schema_to_stdout(struct ArrowSchema* schema, int level, char* buf,
int buf_size) {
- int n_chars = ArrowSchemaToString(schema, buf, buf_size, 0);
Review Comment:
I think this is still needed (or else the top-level type will never appear
in the dump). Maybe just remove `int n_chars = ` (if the unused variable was a
problem?)
##########
src/nanoarrow/meson.build:
##########
@@ -64,6 +64,25 @@ incdir = include_directories('..')
nanoarrow_dep = declare_dependency(include_directories: [curdir, incdir],
link_with: nanoarrow_lib)
+if get_option('ipc')
+ cmake = import('cmake')
+ cmake_opts = cmake.subproject_options()
+ cmake_opts.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': true})
+ flatcc_subproj = cmake.subproject('flatcc', options: cmake_opts)
+ flatcc_dep = flatcc_subproj.dependency('flatccrt')
Review Comment:
This would mean that CMake is required to build the flatcc runtime? I get
that it is more "meson"ic to use the wrap file; however, it might also be a
little strange that building nanoarrow_ipc via CMake will give you a slightly
different result than using Meson. I think I'd prefer that the default build
uses the vendored version (via a pure Meson compile of the four files in
thirdparty/flatcc/src/runtime), and perhaps our dependency relationship with
flatcc could be improved in both CMake/Meson (perhaps with some contributions
to the upstream setup).
##########
dev/release/rat_exclude_files.txt:
##########
@@ -18,3 +18,4 @@ python/src/nanoarrow/dlpack_abi.h
subprojects/google-benchmark.wrap
subprojects/gtest.wrap
subprojects/nlohmann_json.wrap
+subprojects/zlib.wrap
Review Comment:
Is there a reason these are being ignored rather than just adding the Apache
license header to the files? (It seems like those file types support comments
and some of the wrap files already have the license header added)
##########
src/nanoarrow/nanoarrow_ipc_decoder_test.cc:
##########
@@ -602,8 +599,10 @@ TEST(NanoarrowIpcTest,
NanoarrowIpcSharedBufferThreadSafeDecode) {
std::thread threads[10];
for (int i = 0; i < 10; i++) {
threads[i] = std::thread([&arrays, i, &one_two_three_le] {
- memcmp(arrays[i].children[0]->buffers[1], one_two_three_le,
- sizeof(one_two_three_le));
+ auto result = memcmp(arrays[i].children[0]->buffers[1], one_two_three_le,
+ sizeof(one_two_three_le));
+ // discard result to silence -Wunused-value
+ (void)result;
Review Comment:
Would `NANOARROW_UNUSED()` work here?
##########
src/nanoarrow/meson.build:
##########
@@ -147,4 +166,38 @@ if get_option('tests')
include_directories: incdir)
test('c_data_integration test', c_data_integration_test)
+ if get_option('ipc')
+ zlib_dep = dependency('zlib')
+ ipc_test_files = {
+ 'nanoarrow-ipc-decoder': {
+ 'deps': [nanoarrow_dep, flatcc_dep, arrow_dep, gtest_dep],
+ },
+ 'nanoarrow-ipc-reader': {
+ 'deps': [nanoarrow_dep, flatcc_dep, arrow_dep, gtest_dep],
+ },
+ 'nanoarrow-ipc-files': {
+ 'deps': [
+ nanoarrow_dep,
+ flatcc_dep,
+ zlib_dep,
+ arrow_dep,
+ gtest_dep,
+ nlohmann_json_dep
+ ],
+ },
+ 'nanoarrow-ipc-hpp': {
+ 'deps': [nanoarrow_dep, flatcc_dep, gtest_dep],
+ },
+ }
+
+ foreach name, config : ipc_test_files
+ exc = executable(
+ name + '-test',
+ name.replace('-', '_') + '_test.cc',
+ link_with: nanoarrow_ipc_lib,
+ dependencies: config['deps']
Review Comment:
Would it be possible to factor more common dependencies like `gtest_dep`
into `link_with`?
(Also: I don't think that `nanoarrow_dep` or `flatcc_dep` is needed because
it should be handled as a transitive dependency of `nanoarrow_ipc_lib`? I might
have put that in the CMake due to some weirdness about it being a library built
on top of nanoarrow rather than part of nanoarrow itself)
##########
ci/scripts/build-with-meson.sh:
##########
@@ -68,12 +68,12 @@ function main() {
show_header "Run test suite"
meson configure -Dtests=true -Db_coverage=true
meson compile
- meson test --wrap valgrind
+ meson test --wrap='valgrind --track-origins=yes' --print-errorlogs
Review Comment:
I often use `--tool=memcheck --leak-check=full` (but this seems to give
great output so take or leave!)
##########
src/nanoarrow/nanoarrow_testing_test.cc:
##########
@@ -470,7 +470,7 @@ TEST(NanoarrowTestingTest,
NanoarrowTestingTestFieldMetadata) {
NANOARROW_RETURN_NOT_OK(ArrowSchemaSetMetadata(schema, "\0\0\0\0"));
return NANOARROW_OK;
},
- [](ArrowArray* array) { return NANOARROW_OK; }, &WriteFieldJSON,
+ [](ArrowArray*) { return NANOARROW_OK; }, &WriteFieldJSON,
Review Comment:
Makes sense to me!
--
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]
