Skip to content

Conversation

@arghness
Copy link

@arghness arghness commented Dec 9, 2025

These methods can both throw due to ensure_impl() throwing.

Summary by CodeRabbit

  • Breaking Changes
    • Exception-safety guarantees have been removed from two core API methods. These methods may now throw exceptions in scenarios where exceptions were previously guaranteed not to occur. Applications relying on the previous no-throw behavior should be reviewed.

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

These methods can both throw due to ensure_impl() throwing.
@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Walkthrough

Two public methods in the line sender interface had their noexcept specifiers removed: protocol_version() and new_buffer(). This changes their exception-safety guarantees from no-throw to potentially-throwing.

Changes

Cohort / File(s) Summary
Exception-safety specification changes
include/questdb/ingress/line_sender.hpp
Removed noexcept specifier from protocol_version() const and new_buffer(size_t init_buf_size = 64 \* 1024) methods, permitting these methods to throw exceptions

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Note: Verify that callers of these two methods are prepared to handle potential exceptions, and confirm that the removal of noexcept aligns with actual implementation behavior changes in the corresponding .cpp file(s).

Poem

🐰 The noexcept bonds have come undone,
Our methods now may throw with fun,
Exception-safe no more they'll be,
They leap and dance so wild and free!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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 directly and specifically describes the main change in the pull request: removing noexcept from two named methods.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 34905ab and a8ff26a.

📒 Files selected for processing (1)
  • include/questdb/ingress/line_sender.hpp (1 hunks)
⏰ 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). (9)
  • GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
  • GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
  • GitHub Check: questdb.c-questdb-client (Building and testing on mac)
  • GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
  • GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
  • GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
  • GitHub Check: questdb.c-questdb-client (Building and testing on linux)
  • GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
  • GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (2)
include/questdb/ingress/line_sender.hpp (2)

1113-1118: Exception contract now correctly reflects actual behavior.

Removing noexcept from protocol_version() is correct—the method calls ensure_impl() (line 1115), which throws line_sender_error if the sender is closed. The declaration now accurately reflects that this method can throw.

Please verify that callers of protocol_version() either (a) are prepared to handle exceptions or (b) already call ensure_impl() indirectly via other methods that maintain the postcondition that _impl is not null.


1120-1127: Exception contract correctly updated.

Removing noexcept from new_buffer() is justified. While ensure_impl() (line 1122) guards against a closed sender before protocol_version() is called on line 1124, the method signature should declare that it can throw. This accurately represents the method's behavior.


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.

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