Skip to content

[cudax][cuco] Explicitly mark the open addressing destructor as host-only - #11345

Open
PointKernel wants to merge 4 commits into
NVIDIA:mainfrom
PointKernel:fix-cuco-host-destructor
Open

[cudax][cuco] Explicitly mark the open addressing destructor as host-only#11345
PointKernel wants to merge 4 commits into
NVIDIA:mainfrom
PointKernel:fix-cuco-host-destructor

Conversation

@PointKernel

Copy link
Copy Markdown
Member

Description

Follow-up to #11017 and #11340.

This PR explicitly marks the __open_addressing_impl destructor as host-only to prevent nvcc diagnostic 20011 and preserves its existing copy/move behavior. It removes the diagnostic suppression from the existing capacity tests and adds compile-time copy/move checks.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-project-automation github-project-automation Bot moved this to Todo in CCCL Sep 10, 2026
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Progress in CCCL Sep 10, 2026
@PointKernel

Copy link
Copy Markdown
Member Author

/ok to test

@PointKernel PointKernel added the cuco cuCollections label Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 1h 33m: Pass: 100%/63 | Total: 13h 08m | Max: 56m 33s | Hits: 84%/44284

See results here.

@PointKernel
PointKernel marked this pull request as ready for review September 11, 2026 16:27
@PointKernel
PointKernel requested review from a team as code owners September 11, 2026 16:27
@cccl-authenticator-app cccl-authenticator-app Bot moved this from In Progress to In Review in CCCL Sep 11, 2026
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved resource cleanup for device-backed map storage during host-side destruction.
    • Clarified copy and move behavior for map implementations, preventing unsupported copy assignment.
  • Tests

    • Added coverage validating copy and move construction and assignment behavior.
    • Removed a temporary compiler diagnostic suppression from the map tests.

Walkthrough

The open-addressing implementation now declares explicit copy and move special members and a host destructor. The dynamic-capacity map test validates the resulting construction and assignment traits.

Changes

Open-addressing special members

Layer / File(s) Summary
Special-member declarations
cudax/include/cuda/experimental/__cuco/detail/open_addressing/open_addressing_impl.cuh
__open_addressing_impl adds defaulted copy and move constructors, deletes copy assignment, adds move assignment, and defines a host destructor.
Special-member validation
cudax/test/cuco/fixed_capacity_map/test_capacity.cu
The test removes the NVCC workaround, includes the required headers, and adds static assertions for copy and nothrow move properties.

Suggested reviewers: srinivasyadav18

Priority: ⬇️ Low

Change: Bug fix

Merge Risk: 🔵 Low · up to c9c4b

The new special-member API lacks required documentation, reducing clarity for maintainers. Address this small documentation contract issue before merge.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 58e901b2-3a13-4d8f-a7f5-08d6fb86f0b8

📥 Commits

Reviewing files that changed from the base of the PR and between 357b7ec and c9c4b5b.

📒 Files selected for processing (2)
  • cudax/include/cuda/experimental/__cuco/detail/open_addressing/open_addressing_impl.cuh
  • cudax/test/cuco/fixed_capacity_map/test_capacity.cu

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

}

//! @brief Copy-constructs an open addressing implementation.
_CCCL_HIDE_FROM_ABI __open_addressing_impl(const __open_addressing_impl&) = default;

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

important: Add Doxygen parameter documentation for the copy and move operations. Add return documentation for move assignment. Name the parameters so the documentation can identify them.

-  _CCCL_HIDE_FROM_ABI __open_addressing_impl(const __open_addressing_impl&) = default;
+  //! `@param`[in] __other Source implementation.
+  _CCCL_HIDE_FROM_ABI __open_addressing_impl(const __open_addressing_impl& __other) = default;

-  _CCCL_HIDE_FROM_ABI __open_addressing_impl(__open_addressing_impl&&) = default;
+  //! `@param`[in,out] __other Source implementation.
+  _CCCL_HIDE_FROM_ABI __open_addressing_impl(__open_addressing_impl&& __other) = default;

-  _CCCL_HIDE_FROM_ABI __open_addressing_impl& operator=(__open_addressing_impl&&) = default;
+  //! `@param`[in,out] __other Source implementation.
+  //! `@return` Reference to this implementation.
+  _CCCL_HIDE_FROM_ABI __open_addressing_impl& operator=(__open_addressing_impl&& __other) = default;

As per coding guidelines, documented functions must include @param for every parameter and @return for non-void functions.

Also applies to: 231-231, 236-236

Source: Coding guidelines

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

Labels

cuco cuCollections

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants