Skip to content

Fix noexcept specification in qsbr_thread::make_qsbr_thread#805

Merged
laurynas-biveinis merged 1 commit into
masterfrom
fix-qsbr_thread-noexcept
Nov 20, 2025
Merged

Fix noexcept specification in qsbr_thread::make_qsbr_thread#805
laurynas-biveinis merged 1 commit into
masterfrom
fix-qsbr_thread-noexcept

Conversation

@laurynas-biveinis
Copy link
Copy Markdown
Collaborator

@laurynas-biveinis laurynas-biveinis commented Nov 19, 2025

Summary by CodeRabbit

  • Refactor
    • Tightened exception specifications and strengthened argument forwarding for thread startup, preserving runtime behavior while improving compile-time checks.
    • Made test utilities and a thread test's exception specifications conditional on build configuration, aligning test behavior with build-time settings without changing control flow.

✏️ Tip: You can customize this high-level summary in your review settings.

@laurynas-biveinis laurynas-biveinis self-assigned this Nov 19, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 19, 2025

Walkthrough

Tightened forwarding and refined noexcept on the per-thread QSBR start lambda in qsbr.hpp; applied the same conditional noexcept to a test lambda in test/test_qsbr.cpp; added a conditional noexcept annotation to QSBRTestBase::qsbr_pause() in test/qsbr_gtest_utils.hpp.

Changes

Cohort / File(s) Summary
Thread lambda: forwarding & noexcept
qsbr.hpp, test/test_qsbr.cpp
Thread-start lambda now captures renamed params (a2) and forwards them with std::forward<decltype(a2)>(a2)... instead of forwarding Args; the lambda's noexcept is now noexcept(noexcept(f2(std::forward<decltype(a2)>(a2)...))). In the test the noexcept is conditional on UNODB_DETAIL_WITH_STATS.
Test helper: conditional noexcept
test/qsbr_gtest_utils.hpp
QSBRTestBase::qsbr_pause() signature now conditionally includes noexcept via #ifndef UNODB_DETAIL_WITH_STATS / #endif (behavior unchanged).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Main
  participant QSBR as qsbr_thread::make_qsbr_thread
  participant Worker as thread lambda
  participant F as f2

  Main->>QSBR: create thread with callable + args
  QSBR->>Worker: start thread (lambda captures `a2`...)
  Worker->>F: call f2(std::forward<decltype(a2)>(a2)...)
  note right of Worker `#D6EAF8`: lambda noexcept = noexcept(noexcept(f2(...)))\n(test conditional on UNODB_DETAIL_WITH_STATS)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to:
    • Correct preservation of value categories via decltype(a2) forwarding.
    • Effects of the tightened noexcept on exception propagation and overload resolution.
    • Conditional macro UNODB_DETAIL_WITH_STATS in test builds.

Poem

A tiny lambda minding its ways,
Forwards with care through compile-time haze.
Tests stay quiet when stats are mute,
A subtle fix, precise and astute.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing noexcept specifications in qsbr_thread::make_qsbr_thread, which is the primary focus across multiple files in this changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-qsbr_thread-noexcept

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 057a5b8 and 787327e.

📒 Files selected for processing (3)
  • qsbr.hpp (1 hunks)
  • test/qsbr_gtest_utils.hpp (1 hunks)
  • test/test_qsbr.cpp (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/test_qsbr.cpp (1)
test/qsbr_gtest_utils.hpp (1)
  • UNODB_DETAIL_WITH_STATS (229-244)
qsbr.hpp (1)
art_common.hpp (2)
  • noexcept (71-71)
  • noexcept (82-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
test/qsbr_gtest_utils.hpp (1)

59-69: LGTM - Conditional noexcept correctly propagated.

The method signature now properly reflects the conditional noexcept of its implementation. When UNODB_DETAIL_WITH_STATS is not defined, both the method and its internal lambda are noexcept; otherwise, they may throw (presumably from stats-related mutex operations).

qsbr.hpp (1)

594-600: LGTM - Critical noexcept specification fix.

The old noexcept(noexcept(f2)) was incorrect: it checked whether the callable object f2 itself has a noexcept qualifier, not whether invoking f2 is noexcept. The new noexcept(noexcept(f2(std::forward<decltype(a2)>(a2)...))) correctly evaluates the noexcept-ness of the actual function call expression.

The forwarding change from std::forward<Args>(args2)... to std::forward<decltype(a2)>(a2)... is also more appropriate for generic lambda parameters.

test/test_qsbr.cpp (1)

61-65: LGTM - Test lambda noexcept aligned with implementation.

The conditional noexcept specification correctly matches the exception specification of qsbr_pause() that this lambda invokes. This ensures the test accurately reflects the actual exception guarantees of the code under test.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@laurynas-biveinis laurynas-biveinis force-pushed the fix-qsbr_thread-noexcept branch 3 times, most recently from 754567c to 057a5b8 Compare November 20, 2025 09:54
@sonarqubecloud
Copy link
Copy Markdown

@laurynas-biveinis laurynas-biveinis merged commit 595363f into master Nov 20, 2025
233 of 237 checks passed
@laurynas-biveinis laurynas-biveinis deleted the fix-qsbr_thread-noexcept branch November 20, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant