Repository navigation
Apply some clang-tidy-19 suggestions - #496
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies code improvements suggested by clang-tidy-19 to enhance performance and code quality. The changes include passing parameters by const reference where appropriate, adding noexcept specifiers to swap functions, using designated initializers, adding reserve() calls before loop insertions, and modernizing C++ code style.
Key Changes:
- Parameters changed to const references to avoid unnecessary copies
- Added
noexceptto swap functions for better move semantics - Added
reserve()calls to optimize vector allocations - Replaced nested
std::mincalls with initializer list syntax - Updated namespace declarations to modern C++ style
- Added
overridekeyword to virtual destructor
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libbroker/broker/store.test.cc | Changed temporary objects to named variables for default construction test |
| libbroker/broker/radix_tree.test.cc | Pass parameters by const reference, use CHECK_EQUAL with unsigned literals |
| libbroker/broker/peering.test.cc | Pass parameters by const reference, add reserve() for vector optimization |
| libbroker/broker/internal/channel.test.cc | Pass parameters by const reference to avoid copies |
| libbroker/broker/format/bin.test.cc | Pass parameters by const reference, use designated initializers |
| libbroker/broker/detail/radix_tree.hh | Add noexcept to swap functions, modernize namespace syntax, simplify std::min calls |
| libbroker/broker/builder.test.cc | Pass parameter by const reference |
| libbroker/broker/backend.test.cc | Add override keyword and reserve() call |
| libbroker/broker/alm/routing_table.test.cc | Pass parameter by const reference |
| libbroker/broker/alm/multipath.test.cc | Pass parameter by const reference, add reserve() call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
15abddd to
8f89dc8
Compare
timwoj
left a comment
There was a problem hiding this comment.
I wonder if we should update the .clang-tidy configuration for Broker to match the one in Zeek again. It's probably been a while. I wouldn't do it as part of this PR though.
I meant to look into #365 again to see if newer
clang-tidyversions maybe fixed the issue. Turns out ourclang-tidyCI check doesn't do anything anymore since we've moved our sources fromsrctolibbroker. 🤦♂️These are some changes
clang-tidy-19has suggested. Unfortunately, it did suggest changes that break the code, so we can't just update CI and start running this check with 19 again. Suppressing every suggestion that broke the code would add a lot of noise to the code. So before bringing the check back in a future PR, I'll see if I can maybe restructure the code slightly and otherwise disable noisy checks.