Skip to content

Move Buffer::overloaded dispatch helper to top level #238

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

Merged

Conversation

wence-
Copy link
Contributor

@wence- wence- commented May 2, 2025

GCC versions prior to v12.0 had a bug in class template argument deduction rules: they were incorrectly required to live at namespace scope.

For example, the following definition

struct Foo {
  template <typename... Ts> struct overloaded : Ts... {
    using Ts::operator()...;
  };
  template <typename... Ts>
  overloaded(Ts...) -> overloaded<Ts...>;
};

Produces the compile error

error: deduction guide 'Foo::overloaded(Ts ...) -> Foo::overloaded<Ts ...>' must be declared at namespace scope
    6 |   overloaded(Ts...) -> overloaded<Ts...>;
      |   ^~~~~~~~~~

In contrast, both clang, and later versions of GCC, correctly accept this definition. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79501 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100983 for details.

Since we still advertise support for GCC 11, and pip-based devcontainer builds use GCC 11, work around this bug by moving the overloaded dispatch helper (used in methods that visit the variant storage) to a top-level anonymous namespace.

GCC versions prior to v12.0 had a bug in class template argument
deduction rules: they were incorrectly required to live at namespace
scope.

For example, the following definition

    struct Foo {
      template <typename... Ts> struct overloaded : Ts... {
        using Ts::operator()...;
      };
      template <typename... Ts>
      overloaded(Ts...) -> overloaded<Ts...>;
    };

Produces the compile error

    error: deduction guide 'Foo::overloaded(Ts ...) -> Foo::overloaded<Ts ...>' must be declared at namespace scope
        6 |   overloaded(Ts...) -> overloaded<Ts...>;
          |   ^~~~~~~~~~

In contrast, both clang, and later versions of GCC, correctly accept
this definition. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79501 and
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100983 for details.

Since we still advertise support for GCC 11, and pip-based
devcontainer builds use GCC 11, work around this bug by moving the
overloaded dispatch helper (used in methods that visit the variant
storage) to a top-level anonymous namespace.
@wence- wence- requested a review from a team as a code owner May 2, 2025 08:55
@wence- wence- added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 2, 2025
@wence-
Copy link
Contributor Author

wence- commented May 2, 2025

Noticed as part of rapidsai/devcontainers#497

Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

I don't oppose this change, but IIUC this is only required for CUDA 11.x which we explicitly don't support. See conditions where gcc 11.x is required in cuDF versus the only condition rapidsmpf satisfies. To me it seems that the correct solution is to not try to compile rapidsmpf with CUDA 11.x in devcontainers.

Copy link
Contributor

@nirandaperera nirandaperera left a comment

Choose a reason for hiding this comment

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

LGTM

@wence-
Copy link
Contributor Author

wence- commented May 2, 2025

I don't oppose this change, but IIUC this is only required for CUDA 11.x which we explicitly don't support. See conditions where gcc 11.x is required in cuDF versus the only condition rapidsmpf satisfies. To me it seems that the correct solution is to not try to compile rapidsmpf with CUDA 11.x in devcontainers.

This is required for the pip devcontainer which has GCC 11 as a host. The nvcc host compiler requirement for cuda 12.8 is GCC >= 10; cudf needs GCC >= 11.4.

@wence-
Copy link
Contributor Author

wence- commented May 2, 2025

/merge

@rapids-bot rapids-bot bot merged commit d556b30 into rapidsai:branch-25.06 May 2, 2025
21 checks passed
@wence- wence- deleted the wence/fix/gcc-11-workaround branch May 2, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants