Replace the two-way comparison operators in qsbr_ptr with the three-way one#701
Replace the two-way comparison operators in qsbr_ptr with the three-way one#701laurynas-biveinis wants to merge 1 commit into
Conversation
WalkthroughReplaces qsbr_ptr's four binary relational operators with a single C++20 three-way comparison operator ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant L as qsbr_ptr(lhs)
participant R as qsbr_ptr(rhs)
Note over L,R: ordering comparison request
Client->>L: compare_with(R)
L->>R: fetch pointers (get())
L->>R: invoke lhs.get() <=> rhs.get()
alt result
L-->>Client: returns std::strong_ordering / ordering result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
qsbr_ptr.hpp(1 hunks)
🔇 Additional comments (1)
qsbr_ptr.hpp (1)
245-250: LGTM! Clean and modern implementation.The three-way comparison operator is correctly implemented:
- Properly marked as
constexpr,nodiscard, andgnu::pure- Correctly delegates to the raw pointer's spaceship operator
- Parameter passing is appropriate for pointer-sized types
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #701 +/- ##
=======================================
Coverage 98.35% 98.35%
=======================================
Files 33 33
Lines 7043 7044 +1
=======================================
+ Hits 6927 6928 +1
Misses 116 116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
laurynas-biveinis
force-pushed
the
qsbr_ptr-spaceship-operator
branch
from
2083723 to
702bd83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
qsbr_ptr.hpp(2 hunks)test/test_qsbr_ptr.cpp(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). (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 (2)
test/test_qsbr_ptr.cpp (1)
11-11: LGTM!The include is necessary for spaceship operator support, and the IWYU pragma correctly ensures it's retained.
qsbr_ptr.hpp (1)
18-18: LGTM!The include provides the necessary support for the spaceship operator and is correctly placed.
laurynas-biveinis
force-pushed
the
qsbr_ptr-spaceship-operator
branch
4 times, most recently
from
e4ce6f2 to
759d74e
Compare
…ay one Add a workaround for clang 11 (only that version) spurious warning
laurynas-biveinis
force-pushed
the
qsbr_ptr-spaceship-operator
branch
from
759d74e to
1cbc7bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.circleci/config.yml(1 hunks)CMakeLists.txt(3 hunks)qsbr_ptr.hpp(2 hunks)test/test_qsbr_ptr.cpp(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). (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 (6)
CMakeLists.txt (3)
87-89: LGTM.The workaround for clang 11's spurious spaceship operator warning is appropriate. The comment clearly references the upstream LLVM issue.
406-407: LGTM.The condition helpers correctly identify clang 11 on non-Windows platforms. The logic is sound and follows existing patterns.
713-713: LGTM.The warning flag is correctly applied only to standalone builds using clang 11 on non-Windows platforms. The generator expression is properly formed.
test/test_qsbr_ptr.cpp (1)
11-11: LGTM.The
#include <compare>is required for the spaceship operator support. The IWYU pragma is correct to prevent removal.qsbr_ptr.hpp (2)
18-18: LGTM.The
#include <compare>is required for the three-way comparison operator. The IWYU pragma is appropriate.
261-265: LGTM.The three-way comparison operator correctly delegates to the underlying pointer comparison. The implementation is idiomatic C++20, and past review feedback was addressed.
| name: Build | ||
| working_directory: build | ||
| command: make -j2 -k | ||
| command: make -k |
There was a problem hiding this comment.
🧩 Analysis chain
Justify removing parallel builds.
This change removes the -j2 flag, switching from parallel to serial compilation. This will significantly slow down CI builds. The PR objectives mention replacing comparison operators, not build system changes. What's the reason for this? If it's related to compilation issues with the spaceship operator, document it. Otherwise, restore parallelism.
Run the following to check if there were recent build failures related to parallel compilation:
🏁 Script executed:
#!/bin/bash
# Check recent CircleCI builds for parallel compilation issues
gh api repos/laurynas-biveinis/unodb/actions/runs --jq '.workflow_runs[] | select(.head_branch == "master") | {conclusion, created_at, head_sha}' | head -20Length of output: 229
I need more context. Let me check the PR details and the actual changes made:
I need to check the actual PR context and git history for this change:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get git history for the CircleCI config file to see who changed it and why
git log --oneline --all -20 -- .circleci/config.yml 2>/dev/null || echo "Git history unavailable"
# Also try to get the current PR branch info
git log -1 --format='%H %s' 2>/dev/null || echo "No git context available"
Length of output: 48
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the actual CircleCI config around line 155 to confirm the change
sed -n '150,160p' .circleci/config.yml
Length of output: 358
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for -j2 anywhere in the CircleCI config to see if it was used before
rg -n '\-j' .circleci/config.yml
# Also check if there are any comments explaining the build change
rg -n -B2 -A2 'make.*-k' .circleci/config.yml
Length of output: 1672
Restore parallelism to the main build or document why it was removed.
Line 155 is the only main build command in the file without parallelism. Every other build retains it: line 112 uses -j2, line 182 uses -j2, line 248 uses -j3, and line 229 uses -j3. This inconsistency will reduce CI performance without explanation. Either restore -j2 to match the Linux pattern or add a comment explaining why this specific build requires serial compilation.
🤖 Prompt for AI Agents
In .circleci/config.yml around line 155, the main build command uses "make -k"
without parallelism which is inconsistent with other jobs using -j2/-j3; either
restore parallel make by changing the command to "make -k -j2" to match the
Linux pattern, or if parallelism causes flakiness, add a short inline comment
explaining why this job must run serially (e.g., unstable parallel builds or
test ordering) so the inconsistency is documented for future maintainers.
|
1 participant
Summary by CodeRabbit
Refactor
Tests
Chores