SemyonSinchenko commented on code in PR #952: URL: https://github.com/apache/incubator-graphar/pull/952#discussion_r3797015266
########## cpp/src/graphar/fwd.h: ########## @@ -25,6 +25,7 @@ #include <unordered_map> #include <vector> +#include "arrow/util/config.h" Review Comment: Including `arrow/util/config.h` in `fwd.h` couples the most widely included public header to Arrow. `fwd.h` is pulled in by the Arrow-free "info" API chain (`graphar/api/info.h` -> `graph_info.h`/`types.h`/`result.h`), and since the `graphar` CMake target links Arrow PRIVATE and `graphar-config.cmake` does not export Arrow include dirs, downstream users who only consume the info API would now need Arrow dev headers on their include path just to compile. Note that all actual users of `ARROW_VERSION` in this change are `.cc` files (expression.cc, reader_util.cc, chunk_reader.cc, chunk_writer.cc, graph_reader.cc, filesystem.cc) — and `test/util.h` already includes `config.h` itself — so the include is not needed in `fwd.h` for correctness. Consider including `arrow/util/config.h` directly in each translation unit that references `ARROW_VERSION` instead, preserving the Arrow-free nature of the info-level public headers. -- 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]
