refactor: Fix clang-tidy bugprone-empty-catch check - #6419
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses clang-tidy's bugprone-empty-catch check by adding suppressions for intentional empty catch blocks and refactoring code to move return statements into catch blocks. The PR enables the check in .clang-tidy configuration and adds the related suppressions (NOLINT and NOLINTNEXTLINE) to the spell-checker dictionary.
Changes:
- Enabled
bugprone-empty-catchcheck in.clang-tidyconfiguration - Added NOLINT suppressions to 16 intentional empty catch blocks across production and test code
- Refactored 5 functions to move return statements from after catch blocks into the catch blocks themselves
- Added
NOLINTandNOLINTNEXTLINEtocspell.config.yaml
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.clang-tidy |
Enabled bugprone-empty-catch check and moved it from disabled list to enabled list |
cspell.config.yaml |
Added NOLINT and NOLINTNEXTLINE to dictionary for spell-checking |
src/xrpld/rpc/detail/RPCCall.cpp |
Added NOLINT suppression for intentional empty catch in config file handling |
src/xrpld/overlay/detail/ConnectAttempt.cpp |
Added NOLINT suppression for timer cancel error handling |
src/xrpld/app/misc/detail/ValidatorSite.cpp |
Added NOLINT suppressions in 2 places for timer cancel error handling |
src/xrpld/app/main/GRPCServer.cpp |
Added NOLINT suppression for endpoint parsing error handling |
src/xrpld/app/ledger/detail/SkipListAcquire.cpp |
Added NOLINT suppression for data processing error handling |
src/xrpld/app/ledger/detail/LedgerMaster.cpp |
Added NOLINT suppression for missing node error handling |
src/xrpld/app/ledger/detail/InboundLedgers.cpp |
Added NOLINT suppression for node data error handling |
src/libxrpl/nodestore/backend/NuDBFactory.cpp |
Added NOLINT suppression in destructor for close error handling |
src/libxrpl/beast/insight/StatsDCollector.cpp |
Added NOLINT suppression for timer cancel error handling |
src/tests/libxrpl/basics/scope.cpp |
Added NOLINT suppressions in 6 test cases for scope guard testing |
src/test/jtx/impl/WSClient.cpp |
Added NOLINTNEXTLINE suppression for stream cancel error handling |
src/test/jtx/impl/Oracle.cpp |
Moved return statement into catch block for validation function |
src/test/jtx/impl/Env.cpp |
Moved return statements into catch blocks in 2 functions and changed exception type from std::exception const& to ... |
src/test/core/SociDB_test.cpp |
Added NOLINT suppressions in setup and cleanup methods |
src/test/app/Manifest_test.cpp |
Added NOLINT suppressions in setup and cleanup methods |
src/libxrpl/tx/transactors/XChainBridge.cpp |
Moved return statement into existing catch(...) block |
src/libxrpl/protocol/STTx.cpp |
Moved return statement into catch block and changed exception type from std::exception const& to ... |
src/libxrpl/protocol/STAmount.cpp |
Moved return statement into catch block, changed exception type from std::exception const& to ..., and added blank line for readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Probably, it is better to add a |
Copilot makes it sound very scary but in reality i think it's the same thing (yes technically it could change behaviour but i doubt the code throws anything that isn't derived from |
ximinez
left a comment
There was a problem hiding this comment.
All the places that you converted to catch (...) certainly look to me like they were intended as a "catch everything", and only limited it to std::exception because that's what's supposed to be thrown.
This reverts commit 58e429f.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bugprone-empty-catch checkbugprone-empty-catch check
This change fixes or suppresses instances detected by the `bugprone-empty-catch` clang-tidy check.
High Level Overview of Change
Fixes instances of
bugprone-empty-catchof clang-tidy.Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
No impact.