-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-17693: [C++] Remove string_view backport #14177
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
9845b80
to
c9a2044
Compare
This comment was marked as outdated.
This comment was marked as outdated.
c9a2044
to
9f63712
Compare
This comment was marked as outdated.
This comment was marked as outdated.
9f63712
to
3fbeb9a
Compare
62bbdcf
to
f92c450
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@@ -1974,9 +1974,8 @@ macro(build_gtest) | |||
set(dummy ">") | |||
|
|||
set(GTEST_CMAKE_ARGS | |||
${EP_COMMON_TOOLCHAIN} | |||
${EP_COMMON_CMAKE_ARGS} |
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.
This is so that CMAKE_CXX_STANDARD is propagated to the GTest build.
Revision: c7e43c9 Submitted crossbow builds: ursacomputing/crossbow @ string-view-5 |
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
@@ -146,8 +146,7 @@ jobs: | |||
ARROW_WITH_SNAPPY: ON | |||
ARROW_WITH_ZLIB: ON | |||
ARROW_WITH_ZSTD: ON | |||
# System Abseil installed by Homebrew uses C++ 17 | |||
CMAKE_CXX_STANDARD: 17 | |||
GTest_SOURCE: BUNDLED |
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.
Why do we need this? Do we need newer GTest for std::string_view
? If so, how about updating required GTest version too? https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L2078
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.
The GTest version does not need to be new, but GTest needs to be compiled in C++17 mode otherwise some test helpers are needed (such as equality between string_views).
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.
Ah, I see.
Ok, all CI failures are unrelated, I'll merge. |
Benchmark runs are scheduled for baseline = afd3c40 and contender = 91ee6da. 91ee6da is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
No description provided.