-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-45885: [C++] Require C++20 #48414
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
Conversation
|
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit wheelcp313* |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit -g python |
|
@github-actions crossbow submit -g r |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1c42352 to
e654d71
Compare
|
@github-actions crossbow submit -g c-glib -g ruby -g r |
|
Revision: 9d8b52f Submitted crossbow builds: ursacomputing/crossbow @ actions-037a71b82f
|
|
@github-actions crossbow submit -g verify-rc |
|
Revision: 9d8b52f Submitted crossbow builds: ursacomputing/crossbow @ actions-264dd4e180 |
4c8506b to
42fbedc
Compare
zanmato1984
left a comment
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
|
I think all review comments have been addressed, have I overlooked something @kou @WillAyd @zanmato1984 ? |
42fbedc to
6f165ed
Compare
|
@github-actions crossbow submit -g r -g cpp -g python |
|
Revision: 6f165ed Submitted crossbow builds: ursacomputing/crossbow @ actions-370fa26cd6 |
6f165ed to
6fa77b0
Compare
|
I'm not sure what it's for, but I've re-enabled the |
|
@github-actions crossbow submit test-debian-experimental-cpp-gcc-15 |
|
Revision: 6fa77b0 Submitted crossbow builds: ursacomputing/crossbow @ actions-93c23edec8
|
|
@github-actions crossbow submit -g cpp -g python |
|
Revision: 6fa77b0 Submitted crossbow builds: ursacomputing/crossbow @ actions-e5d2f93e4c |
|
Well, let's merge this now. |
|
|
||
| foreach(LIB_TARGET ${ARROW_LIBRARIES}) | ||
| target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_EXPORTING) | ||
| # C++17 is required to compile against Arrow C++ headers and libraries |
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.
Could you update this "C++17" too?
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.
Oops, sorry, it's intriguing that I overlooked these two occurrences.
| INCLUDE_DIRS ${MATLAB_ARROW_LIBMEXCLASS_CLIENT_PROXY_LIBRARY_INCLUDE_DIRS} | ||
| LINK_LIBRARIES arrow_shared | ||
| ) | ||
| # Use C++17 |
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.
Could you update this too?
It's for the latest GCC. We should bump this GCC version when new GCC is released. Debian experimental will provide it. For example, GCC 16 isn't released yet but it's already provided: https://packages.debian.org/search?keywords=gcc-16 |
### Rationale for this change As a followup to #48414, fix the comments that I had forgotten to update. ### Are these changes tested? No testing required. ### Are there any user-facing changes? No. Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Nic Crane <thisisnic@gmail.com>
### Rationale for this change As a followup to apache#48414, fix the comments that I had forgotten to update. ### Are these changes tested? No testing required. ### Are there any user-facing changes? No. Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Nic Crane <thisisnic@gmail.com>
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 30809c6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 474 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
We decided to migrate Arrow C++ to C++20 in this discussion: https://lists.apache.org/thread/48zlj0dn2y0f53y2k37qsr90y781wfnj
What changes are included in this PR?
Build configuration updates (CMake files etc.) to build with C++20 instead of C++17
C++-level fixes to ensure compilation succeeds:
std::shared_ptr<T>(the replacementstd::atomic<std::shared_ptr<T>>is unfortunately not supported in all standard library implementations)arrow/util/string.hto call a C++20 API (to validate that C++20 is actually enabled)CI configuration updates to get enough C++20 support on the various platforms:
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, Arrow C++ will now require a C++20-compliant compiler.