Skip to content

GH-47483: [C++] Bump vendored xxhash to 0.8.3 #47476

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

Merged
pitrou merged 2 commits into from
Sep 3, 2025
Merged

GH-47483: [C++] Bump vendored xxhash to 0.8.3 #47476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29e3ab6
GH-47475: [C++] Bump vendored xxhash to 0.8.3
raulcd Sep 2, 2025
eb76ca8
Add -DXXH_NO_INLINE_HINTS when compiling with -Og
raulcd Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,12 @@ endif()

include(SetupCxxFlags)

if(${CMAKE_CXX_FLAGS_DEBUG} MATCHES "-Og")
Copy link
Copy Markdown
Member

@pitrou pitrou Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kou Does it look like the right way to do this?

Copy link
Copy Markdown
Member

@kou kou Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

In general, we should not use add_definitions() because its scope is global. We should create a target and use it instead:

add_library(arrow::xxhash INTERFACE IMPORTED)
target_compile_definitions(arrow::xxhash INTERFACE "$<$<CONFIG:Debug>:XXH_NO_INLINE_HINTS>")

(I think that we don't need -Og check. We can always disable inline hints in debug build.)

Copy link
Copy Markdown
Member

@kou kou Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: #47495 is an approach that doesn't use add_definitions().

Copy link
Copy Markdown
Member Author

@raulcd raulcd Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kou , should we create targets for the different vendored libraries and only link when necessary as a future task too? I am thinking about datetime, double-conversion, etc

Copy link
Copy Markdown
Member

@kou kou Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes when they need additional build flags. If they don't need additional build flags, we don't need to create targets for them. Because we have src/ in include paths:

arrow/cpp/CMakeLists.txt

Lines 559 to 560 in 743d0af

include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)

BTW, we should not use include_directories() because its scope is global (directory) too...

# GH-47475: xxhash fails inlining when -Og is used.
# See: https://github.com/Cyan4973/xxHash/issues/943
add_definitions(-DXXH_NO_INLINE_HINTS)
endif()

#
# Linker flags
#
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/vendored/xxhash/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@
under the License.
-->

The files in this directory are vendored from xxHash git tag v0.8.2
The files in this directory are vendored from xxHash git tag v0.8.3
(https://github.com/Cyan4973/xxHash).
7 changes: 3 additions & 4 deletions cpp/src/arrow/vendored/xxhash/xxhash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* xxHash - Extremely Fast Hash algorithm
* Copyright (C) 2012-2021 Yann Collet
* Copyright (C) 2012-2023 Yann Collet
*
* BSD 2-Clause License (https://www.opensource.org/licenses/bsd-license.php)
*
Expand Down Expand Up @@ -32,12 +32,11 @@
* - xxHash source repository: https://github.com/Cyan4973/xxHash
*/


/*
* xxhash.c instantiates functions defined in xxhash.h
*/

#define XXH_STATIC_LINKING_ONLY /* access advanced declarations */
#define XXH_IMPLEMENTATION /* access definitions */
#define XXH_STATIC_LINKING_ONLY /* access advanced declarations */
#define XXH_IMPLEMENTATION /* access definitions */

#include "xxhash.h"
Loading
Loading