Skip to content

GH-15281: [C++] Replace bytes_view alias with span - #36334

Merged
pitrou merged 10 commits into
apache:mainfrom
bkietz:15281-replace-bytes-view-with-span
Jul 11, 2023
Merged

GH-15281: [C++] Replace bytes_view alias with span#36334
pitrou merged 10 commits into
apache:mainfrom
bkietz:15281-replace-bytes-view-with-span

Conversation

@bkietz

@bkietzbkietz commented Jun 27, 2023

Copy link
Copy Markdown
Member

Rationale for this change

basic_string_view was never really intended to support types other than chars; the template which the STL intends for use in this situation is std::span

What changes are included in this PR?

Since std::span is added in c++20, a minimal polyfill is added named util::span.

  • doesn't include support for static extents
  • some accessors like front() are not present in util::span

User facing changes:

  • The broken alias bytes_view and its header are removed. I don't expect this to trouble anybody much since it isn't used in much of our API, and if it's really necessary it will be trivial to add it back with using bytes_view = span<uint8_t const>; or so

@pitrou

Copy link
Copy Markdown
Member

@benibus Do you want to review this?

@pitrou

Copy link
Copy Markdown
Member

Should also close #29678.

@bkietz

Copy link
Copy Markdown
MemberAuthor

To workaround #36379 for this PR I could add an overload of FormatMatcherDescription which forwards to the v1.11 one when necessary but won't be called otherwise. However that seems pretty hacky. Please advise

@pitrou

Copy link
Copy Markdown
Member

Let's not overengineer it. You just want to check the pointer/length of a span, so you can write a trivial helper function AssertSpansEqual (or AssertSpansIdentical if you want to stress that it's not a logical comparison of the underlying values).

@bkietz
bkietz requested a review from felipecrvJune 29, 2023 16:47
Comment threadcpp/src/arrow/util/span.h Outdated

@felipecrvfelipecrv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bkietz you remove an existing public header. Shouldn't that be mentioned in the PR description as an user-facing change?

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Jun 29, 2023

@benibusbenibus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this will be useful! In general, LGTM - just one nit.

Comment threadcpp/src/arrow/util/bitmap.h Outdated
Comment threadcpp/src/arrow/util/span.h Outdated
Comment threadcpp/src/arrow/util/span.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be R&&?

@bkietzbkietzJul 6, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Alright, I'll add a more tests to check construction cases against std::span

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Jul 6, 2023
Comment threadcpp/src/arrow/util/span.h Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Jul 10, 2023
@pitrou

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actions

This comment was marked as outdated.

@pitrou
pitrouforce-pushed the 15281-replace-bytes-view-with-span branch from 9d122c1 to 9ce75a1CompareJuly 11, 2023 06:42
@pitrou

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 11, 2023
@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actions

Copy link
Copy Markdown

Revision: 2e3a9eb

Submitted crossbow builds: ursacomputing/crossbow @ actions-972a30de43

TaskStatus
test-alpine-linux-cppGithub Actions
test-build-cpp-fuzzGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-cuda-cppGithub Actions
test-debian-11-cpp-amd64Github Actions
test-debian-11-cpp-i386Github Actions
test-fedora-35-cppGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-bundledGithub Actions
test-ubuntu-20.04-cpp-minimal-with-formatsGithub Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-22.04-cppGithub Actions
test-ubuntu-22.04-cpp-20Github Actions

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Made a small improvement to the tests, I'll merge now. Thank you @bkietz !

@pitrou
pitrou merged commit 366dbe1 into apache:mainJul 11, 2023
@pitroupitrou removed the awaiting change review Awaiting change review label Jul 11, 2023
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 366dbe1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] basic_string_view<long> will be invalid in future libc++

4 participants

@bkietz@pitrou@felipecrv@benibus