ARROW-16609: [C++] xxhash not installed into dist/lib/include when building C++ #13282
Conversation
|
Renaming What functions do you want? Can we move only them to |
|
A file (to be moved out of C++)
I do not mind moving only these parts to |
|
How about moving only them and |
I do not mind. @pitrou can you also share you thoughts on this? |
|
Judging by the comment:
We don't want to move these functions to |
cpp/src/arrow/util/int_util.h
Outdated
| #include <type_traits> | ||
|
|
||
| #include "arrow/status.h" | ||
| #include "arrow/util/int_util_overflow.h" |
There was a problem hiding this comment.
I don't know why we should not include <intsafe.h> in our public headers but we should not include it here.
Only needed python/**/*.cc in #13311 should include it instead.
There was a problem hiding this comment.
AFAIR, intsafe.h includes Windows.h.
There was a problem hiding this comment.
I am not sure I understand the comments, could you please clarify?
To note why arrow/util/int_util_overflow.h is included here: CheckSliceParams which is now moved to int_util.h needs AddWithOverflow from arrow/util/int_util_overflow.h.
There was a problem hiding this comment.
Hmm, then CheckSliceParams can go to int_util_overflow.h probably?
What I meant above is that we don't want Windows.h to be included from our public headers, even transitively.
There was a problem hiding this comment.
But currently this PR makes int_util_overflow.h (formerly int_util_internal.h) also a public header? (and so Windows.h gets included that way as well)
There was a problem hiding this comment.
Well, we have to distribute it somehow for PyArrow (or duplicate the functionality, as was suggested elsewhere, but that doesn't sound ideal). However, as long as it's not included from other public headers, then it's unlikely to be a problem.
There was a problem hiding this comment.
How about creating cpp/src/arrow/util/slice_util_internal.h (or something) and move CheckSliceParams to the header instead of moving CheckSliceParams to int_util_overflow.h? I feel that moving CheckSliceParams to int_util_overflow.h is strange. Because CheckSliceParams is a validation function for Slice. It's not (almost) related to integer overflow.
4 participants
This PR changes CMakeLists .txt files so that
xxhash,portable-snippetsare also installed. Plus it changes the name ofint_util_internal.htoint_util_overflow.hfor the same reason.This step is needed for Python code to be moved to PyArrow in https://issues.apache.org/jira/browse/ARROW-16340.