Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[C++] Conflict between arrow::compute::MemAllocation::PREALLOCATE and system header vnode.h preprocessor define #38709

Closed
cholmcc opened this issue Nov 14, 2023 · 4 comments · Fixed by #38760
Closed

[C++] Conflict between arrow::compute::MemAllocation::PREALLOCATE and system header vnode.h preprocessor define #38709

cholmcc opened this issue Nov 14, 2023 · 4 comments · Fixed by #38760
Assignees
@cholmcc
Labels
Component: C++ Type: bug
Milestone
15.0.0

Comments

@cholmcc
Copy link
Contributor

cholmcc commented Nov 14, 2023

Describe the bug, including details regarding any error messages, version, and platform.

Hi,

On MacOSX the system header vnode.h defines the symbol PREALLOCATE as a preprocessor define. This causes a compilation error with #include <arrow/compute/kernel.h> where this name (PREALLOCATE) is used as an identifier , if sys/vnode.h and arrow/compute/kernel.h are both (possibly indirectly) included into the same compilation unit.

Perhaps the PREALLOCATE symbol should be #undef in arrow/compute/kernel.h in case it is defined as preprocessor macro - e.g.,

    #if defined(__APPLE__) and defined(PREALLOCATE)
    # undef PREALLOCATE 
    #endif

Yours,
Christian

Component(s)

C++
This was referenced Nov 14, 2023
Merged
Open
Merged
@kou kou changed the title Conflict between arrow::compute::MemAllocation::PREALLOCATE and system header vnode.h preprocessor define [C++] Conflict between arrow::compute::MemAllocation::PREALLOCATE and system header vnode.h preprocessor define Nov 14, 2023
@kou
Copy link
Member

kou commented Nov 16, 2023

Could you open a pull request with the approach?

Or we can use kPreallocate name because our coding style uses the convention: https://google.github.io/styleguide/cppguide.html#Enumerator_Names
But it breaks backward compatibility and our many existing source codes don't follow the convention...

@cholmcc
Copy link
Contributor Author

cholmcc commented Nov 16, 2023

Hi,

I think I would suggest to use the kPreallocate approach, as it is much less likely to cause conflicts. However, I would worry a little about backward compatibility with existing client code.

I made a PR. If you choose the kPreallocate route, then just kill the PR.

Yours,
Christian

@kou
Copy link
Member

kou commented Nov 17, 2023

Let's choose undef approach for now.

It seems that a PR isn't made yet. Could you ensure opening a PR with the change?

@cholmcc
Copy link
Contributor Author

cholmcc commented Nov 17, 2023

Sorry, my bad - forgot to hit the button :-) The PR is here

@github-actions github-actions bot mentioned this issue Nov 17, 2023
Merged
kou added a commit that referenced this issue Nov 29, 2023
@cholmcc @kou
GH-38709: [C++] Protect against PREALLOCATE preprocessor defined on m…
61a4a80 
…acOS (#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: #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>
@kou kou added this to the 15.0.0 milestone Nov 29, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@cholmcc @kou @dgreiss
apacheGH-38709: [C++] Protect against PREALLOCATE preprocessor define…
dd9d37e 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cholmcc cholmcc

Labels
Component: C++ Type: bug
Projects
None yet
Milestone
15.0.0
Development

Successfully merging a pull request may close this issue.

GH-38709: [C++] Protect against PREALLOCATE preprocessor defined on macOS cholmcc/arrow
2 participants
@kou @cholmcc