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.

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

GH-45816: [C++] Make VisitType() fallback branch unreachable #45815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Mar 17, 2025

Rationale for this change

Enable using the more convenient constexpr type checking functions from type_traits with VisitType dispatchers, like:

  auto handle_type = [&](auto&& type) {
    using Type = std::decay_t<decltype(type)>;
    if constexpr (::arrow::is_boolean(Type::type_id)) {
      ...
    }
    else if constexpr (::arrow::is_primitive(Type::type_id)) {
        ...
    }
    /* else etc. */
  };
  return VisitType(*values.type(), handle_type);

What changes are included in this PR?

Remove the fallback call with const DataType& rather make the default branch unreachable at compile time.

Are these changes tested?

Yes, VisitType is being used at multiple places in the codebase.

Are there any user-facing changes?

Yes, this is a breaking change. The visitor passed to VisitType doesn't need to have an implementation for the base DataType.

@kszucs kszucs changed the title [C++] Make VisitType() fallback branch unreachable GH-45816: [C++] Make VisitType() fallback branch unreachable Mar 17, 2025
@apache apache deleted a comment from github-actions bot Mar 17, 2025
Copy link

⚠️ GitHub issue #45816 has been automatically assigned in GitHub to PR creator.

@pitrou
Copy link
Member

pitrou commented Mar 17, 2025

I think this is a good idea, let's see if it breaks CI.

@pitrou
Copy link
Member

pitrou commented Mar 17, 2025

@github-actions crossbow submit -g cpp

@pitrou pitrou requested a review from bkietz March 17, 2025 12:27
Copy link

Revision: 0677196

Submitted crossbow builds: ursacomputing/crossbow @ actions-7723444ace

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-meson GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@kszucs kszucs marked this pull request as ready for review March 17, 2025 12:47
@kszucs
Copy link
Member Author

kszucs commented Mar 17, 2025

Shall we explicitly test the following use case?

auto handle_type = [&](auto&& type) {
    using Type = std::decay_t<decltype(type)>;
    if constexpr (::arrow::is_boolean(Type::type_id)) {
      ...
    }
    else if constexpr (::arrow::is_primitive(Type::type_id)) {
        ...
    }
    /* else etc. */
  };
  return VisitType(*values.type(), handle_type);

If yes, then where? Perhaps modify an existing use case, like using type_id based type checkers in arrow::csv::QuotedColumnPopulator::MakePopulator?

@pitrou
Copy link
Member

pitrou commented Mar 17, 2025

IMHO we don't have to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants