Skip to content

misc-throw-by-value-catch-by-reference - #11151

Open
Jacobfaib wants to merge 1 commit into
NVIDIA:mainfrom
Jacobfaib:jacobf/2026-09-02/misc-throw-by-value-catch-by-reference
Open

misc-throw-by-value-catch-by-reference#11151
Jacobfaib wants to merge 1 commit into
NVIDIA:mainfrom
Jacobfaib:jacobf/2026-09-02/misc-throw-by-value-catch-by-reference

Conversation

@Jacobfaib

Copy link
Copy Markdown
Contributor

Fixes all clang-tidy warnings for misc-throw-by-value-catch-by-reference

@Jacobfaib Jacobfaib self-assigned this Sep 2, 2026
@Jacobfaib
Jacobfaib requested a review from a team as a code owner September 2, 2026 22:19
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Sep 2, 2026
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Sep 2, 2026
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bdad8337-d49b-445a-ba0a-1d7e30f60984

📥 Commits

Reviewing files that changed from the base of the PR and between dd688b3 and c79e992.

📒 Files selected for processing (1)
  • .clang-tidy

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


📝 Summary

Summary by CodeRabbit

  • Tests
    • Improved assertion failure handling with clearer propagation of failure details and more consistent diagnostics.
    • Enhanced support for scoped assertion scenarios while preserving existing status reporting.
    • Updated test failure construction and reporting for more reliable diagnostics.
  • Code Quality
    • Updated static-analysis settings and documented rationale for retained check suppressions, including compatibility requirements.

Walkthrough

Changes

Unit test assertion helpers now construct failure objects in named locals and throw them with cuda::std::move. The clang-tidy configuration removes early suppressions and documents the retained misc-multiple-inheritance exclusion.

Unit test exception cleanup

Layer / File(s) Summary
Direct assertion failure construction
thrust/testing/unittest/assertions.h
Assertion failure paths populate local diagnostic objects before throwing them with cuda::std::move. check_assert_throws preserves status handling and diagnostic messages.
Clang-tidy check configuration
.clang-tidy
The configuration removes early suppressions for four checks and retains misc-multiple-inheritance in the disabled-check section with an explanation covering empty-base optimization, CRTP mixins, and ABI requirements.

Merge Risk: ⚪ Minimal · up to c79e9

The Clang-Tidy configuration retains the intended multiple-inheritance exemption while removing unrelated early suppressions; no current merge-readiness risk is identified.


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

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 6h 33m: Pass: 100%/69 | Total: 2d 23h | Max: 2h 00m | Hits: 54%/323960

See results here.

@bernhardmgruber bernhardmgruber 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.

I think the changes in this PR do not improve code readability. I find the new version harder to comprehend, since it now requires ref-qualified stream put operators.

@miscco

miscco commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

I am with @bernhardmgruber I prefer the previous version

@Jacobfaib
Jacobfaib force-pushed the jacobf/2026-09-02/misc-throw-by-value-catch-by-reference branch from c42fc84 to a1f3a20 Compare September 3, 2026 11:07
@Jacobfaib

Copy link
Copy Markdown
Contributor Author

@miscco @bernhardmgruber Another option to fix this is to move the objects into the throw expression:

Foo f;

f << "asdasdasd";
throw std::move(f);

Would that be preferable?

@Jacobfaib
Jacobfaib force-pushed the jacobf/2026-09-02/misc-throw-by-value-catch-by-reference branch 2 times, most recently from dd688b3 to b162389 Compare September 3, 2026 15:20
@Jacobfaib
Jacobfaib force-pushed the jacobf/2026-09-02/misc-throw-by-value-catch-by-reference branch from b162389 to c79e992 Compare September 3, 2026 15:21

@griwes griwes 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.

That's a much nicer fix than the one before.

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

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

4 participants