Continue C++20 modernization #688
Conversation
WalkthroughThis update removes a template constraint from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant qsbr_thread
User->>qsbr_thread: Call constructor with function & arguments
qsbr_thread-->>User: Instance created (no constraint enforced)
sequenceDiagram
participant Test
participant tree_verifier
participant Database
Test->>tree_verifier: Invoke do_insert(key, value)
alt Db is non-olc type
tree_verifier->>Database: Perform insertion (non-olc branch)
else Db is olc type
tree_verifier->>Database: Perform insertion (olc branch)
end
Database-->>tree_verifier: Confirmation
tree_verifier-->>Test: Return result
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🔇 Additional comments (13)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
laurynas-biveinis
force-pushed
the
c++20-requires
branch
from
1a89f5f to
f91e3b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
qsbr.hpp(1 hunks)test/db_test_utils.hpp(9 hunks)
🔇 Additional comments (5)
test/db_test_utils.hpp (4)
16-16: No functional change in IWYU pragma
This line merely updates the IWYU directive; it does not affect functionality or compilation.
222-232: Clean transition to requires
Using requires here instead of template specializations clarifies intent and modernizes the code. Looks good.
340-361: Consistently applied requires-based overloads
All overloads split logically for olc_db and non-olc_db. No issues found.
383-392: Requires usage for try_get
The pattern matches other methods—quiescent state handling in olc_db only. This is consistent and clear.qsbr.hpp (1)
1245-1247: Modern requires clause
Switching from std::enable_if_t to a requires clause simplifies the constructor constraints and is more readable in C++20. No issues found.
laurynas-biveinis
force-pushed
the
c++20-requires
branch
2 times, most recently
from
efdabde to
863f7c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
qsbr.hpp(1 hunks)test/db_test_utils.hpp(10 hunks)
🔇 Additional comments (14)
test/db_test_utils.hpp (13)
16-16: No issues with the updated IWYU pragma.
This line merely adjusts a no-include directive; no action required.
91-94: Good modernization to std::ranges::equal.
Switching from std::equal to std::ranges::equal is consistent with C++20 features and clarifies behavior for range-based comparisons.
146-149: Redundant comment about eventual use of requires.
163-163: No concerns with ending the linter suppress directive.
222-222: No issues with closing preprocessor directive.
223-227: Repeats earlier mention of migrating to requires.
348-349: Modernization: C++20 requires constraint.
Transitioning from std::enable_if_t to requires for remove(...) clarifies the intended preconditions.
353-354: Consistent requires-based overload.
The second remove(...) overload correctly applies the opposite constraint for OLC databases.
359-360: try_remove(...) constraint alignment.
Using requires(!std::is_same_v<Db, unodb::olc_db<key_type>>) here matches the style established above.
364-365: OLC-specific try_remove(...) overload.
Declaring requires(std::is_same_v<Db, unodb::olc_db<key_type>>) for the OLC path maintains consistency.
391-392: Modern constraints for try_get(...)
Applying requires(!std::is_same_v<Db, unodb::olc_db<key_type>>) ensures the method is hidden for OLC DB types.
396-397: OLC specialization for try_get(...)
Overloading with requires(std::is_same_v<Db, unodb::olc_db<key_type>>) follows the pattern for specialized functions.
482-483: Deleted copy/assignment for tree_verifier.qsbr.hpp (1)
1242-1244: Using C++20 requires for constructor constraints.
Replacing std::enable_if_t with requires improves readability and aligns with modern C++ guidelines.
laurynas-biveinis
force-pushed
the
c++20-requires
branch
from
863f7c8 to
8c8cb56
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/db_test_utils.hpp (1)
38-38: Verify necessity of "art_internal.hpp".
It’s unclear if entities from"art_internal.hpp"are used in this file. Consider removing this include if it’s no longer needed.- #include "art_internal.hpp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
qsbr.hpp(1 hunks)test/db_test_utils.hpp(11 hunks)
🔇 Additional comments (14)
test/db_test_utils.hpp (13)
31-31: Header usage looks good.
<gmock/gmock.h>is actively used by matchers like::testing::ElementsAreArrayin this file, so this inclusion is justified.
91-91: Excellent adoption ofstd::ranges::equal.
Ensure your build environment includes<ranges>or that your standard library’s version of<algorithm>has the C++20<ranges>facility.
94-94: Same note onstd::ranges::equal.
Confirm that the necessary C++20 headers are available.
146-149: Reference to replacingstd::enable_if_twithrequires.
This duplicates a past comment recommending a switch to C++20 constraints. Thanks for adding a note about planned migration.
223-227: Repeated plan for migrating torequires.
A similar note applies here regarding eventual replacement ofstd::enable_if_t.
240-240: NOLINT block end detected.
No concerns; the comment block is clearly marked.
348-352: Repeated plan for migrating these methods torequires.
Again, this comment mirrors prior references to future C++20 changes.
365-365: NOLINT block end detected.
No issues; the code style suppression is neatly closed.
367-368: Well-structuredrequiresusage.
Replacing SFINAE with constraints clarifies intent and cleans up the signature.
372-374: Consistentrequiresusage fortry_remove.
This follows the same pattern as above, adding clarity to method constraints.
399-400: Use ofrequiresimproves readability.
Eliminates the need for repetitive template boilerplate and is consistent with C++20 style.
404-405: C++20requiresagain well-applied.
Maintaining two overloads with distinct conditions is more transparent thanenable_if.
490-492: Copy constructor & assignment operator now deleted.
This addresses prior feedback about making non-copyable intentions explicit.qsbr.hpp (1)
1242-1245: Constructor now uses arequiresclause.
Great modernization to enforce thatqsbr_threadcannot construct itself with the same type, simplifying template logic.
laurynas-biveinis
force-pushed
the
c++20-requires
branch
from
8c8cb56 to
ec94d78
Compare
laurynas-biveinis
added
bug
- qsbr.hpp: replace std::enable_if_t with requires - db_test_utils.hpp: comment on replacing std::enable_if_t with requires once - LLVM 15 is the oldest supported LLVM version, replace iterator pairs with ranges, update includes, fix clang-tidy warning on a const field.
laurynas-biveinis
force-pushed
the
c++20-requires
branch
from
ec94d78 to
6127fc7
Compare
|
with ranges, update includes, fix clang-tidy warning on const field.
Summary by CodeRabbit
Refactor
Tests