-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-38709: [C++] Protect against PREALLOCATE preprocessor defined on macOS #38760
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Sidenote: this is why we should discourage ENUM_VALUE in favor of |
Oh, I forgot to merge this. |
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 61a4a80. There were 2 benchmark results indicating a performance regression:
The full Conbench report has more details. |
…d on macOS (apache#38760) ### Rationale for this change The macOS header `sys/vnode.h` defines `PREALLOCATE` as a preprocessor macro, which causes a conflict in `arrow/compute/kernel.h` where the same name is defined as an identifier. Other BSDs does not seem to define this macro. ### What changes are included in this PR? `#undef PREALLOCATE` on macOS and if defined. ### Are these changes tested? Somewhat, in a different context. ### Are there any user-facing changes? If some code specific to macOS actually uses the `PREALLOCATE` macro, then that code may have problems. However, it is unlikely that any user code would use this macro as it seems to be intended for internal use in the BSD kernel of macOS. * Closes: apache#38709 Lead-authored-by: Christian Holm Christensen <Christian.Holm.Christensen@cern.ch> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
The macOS header
sys/vnode.h
definesPREALLOCATE
as a preprocessor macro, which causes a conflict inarrow/compute/kernel.h
where the same name is defined as an identifier.Other BSDs does not seem to define this macro.
What changes are included in this PR?
#undef PREALLOCATE
on macOS and if defined.Are these changes tested?
Somewhat, in a different context.
Are there any user-facing changes?
If some code specific to macOS actually uses the
PREALLOCATE
macro, then that code may have problems. However, it is unlikely that any user code would use this macro as it seems to be intended for internal use in the BSD kernel of macOS.arrow::compute::MemAllocation::PREALLOCATE
and system headervnode.h
preprocessor define #38709