Copilot commented on code in PR #863:
URL: https://github.com/apache/incubator-graphar/pull/863#discussion_r2844147577
##########
cpp/src/graphar/arrow/chunk_reader.cc:
##########
@@ -549,6 +549,23 @@ AdjListArrowChunkReader::AdjListArrowChunkReader(
base_dir_(other.base_dir_),
fs_(other.fs_) {}
+AdjListArrowChunkReader& AdjListArrowChunkReader::operator=(
+ const AdjListArrowChunkReader& other) {
+ if (this != &other) {
+ edge_info_ = other.edge_info_;
+ adj_list_type_ = other.adj_list_type_;
+ vertex_chunk_index_ = other.vertex_chunk_index_;
+ chunk_index_ = other.chunk_index_;
+ seek_offset_ = other.seek_offset_;
+ chunk_table_ = nullptr;
+ vertex_chunk_num_ = other.vertex_chunk_num_;
+ chunk_num_ = other.chunk_num_;
+ base_dir_ = other.base_dir_;
+ fs_ = other.fs_;
Review Comment:
AdjListArrowChunkReader::operator=() does not copy prefix_ (but copy-ctor
does). After assignment, prefix_ can be stale while base_dir_/fs_ refer to the
new object, and methods like seek_src/seek_dst use prefix_ to compute paths,
leading to incorrect reads. Copy prefix_ as well (and keep base_dir_ consistent
with it).
##########
cpp/CMakeLists.txt:
##########
@@ -297,7 +308,7 @@ macro(build_graphar)
install_graphar_target(graphar)
target_compile_features(graphar PRIVATE cxx_std_17)
target_include_directories(graphar PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/thirdparty)
- target_link_libraries(graphar PRIVATE ${CMAKE_DL_LIBS})
+ target_link_libraries(graphar PRIVATE graphar_thirdparty ${CMAKE_DL_LIBS})
Review Comment:
graphar links graphar_thirdparty as PRIVATE. When GRAPHAR_BUILD_STATIC=ON,
PRIVATE link dependencies are not propagated to consumers, so executables/tests
that only link "graphar" can fail with undefined symbols from mini-yaml.
Consider making graphar_thirdparty (and any required system libs like
${CMAKE_DL_LIBS}) PUBLIC/INTERFACE when building graphar as a static library,
while keeping them PRIVATE for the shared build to avoid duplicate symbol
issues.
```suggestion
if(GRAPHAR_BUILD_STATIC)
target_link_libraries(graphar PUBLIC graphar_thirdparty
${CMAKE_DL_LIBS})
else()
target_link_libraries(graphar PRIVATE graphar_thirdparty
${CMAKE_DL_LIBS})
endif()
```
##########
cpp/CMakeLists.txt:
##########
@@ -355,7 +367,7 @@ macro(build_graphar_with_arrow_bundled)
target_compile_features(graphar PRIVATE cxx_std_17)
target_include_directories(graphar PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/thirdparty)
target_include_directories(graphar SYSTEM BEFORE PRIVATE
${GAR_ARROW_INCLUDE_DIR})
- target_link_libraries(graphar PRIVATE ${CMAKE_DL_LIBS})
+ target_link_libraries(graphar PRIVATE graphar_thirdparty ${CMAKE_DL_LIBS})
Review Comment:
Same issue as the non-bundled build: graphar links graphar_thirdparty as
PRIVATE. With GRAPHAR_BUILD_STATIC=ON, consumers linking only "graphar" won't
get graphar_thirdparty on the link line and may hit undefined symbols. Make the
dependency PUBLIC/INTERFACE for the static build (and keep PRIVATE for shared
if desired).
```suggestion
if(GRAPHAR_BUILD_STATIC)
target_link_libraries(graphar PUBLIC graphar_thirdparty
${CMAKE_DL_LIBS})
else()
target_link_libraries(graphar PRIVATE graphar_thirdparty
${CMAKE_DL_LIBS})
endif()
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
