Skip to content

Replace the two-way comparison operators in qsbr_ptr with the three-way one #701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
laurynas-biveinis wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace the two-way comparison operators in qsbr_ptr with the three-way one #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ jobs:
- run:
name: Build
working_directory: build
command: make -j2 -k
command: make -k
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -20

Length 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.

- run:
name: Correctness test
working_directory: build
Expand Down
6 changes: 6 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ set(CLANG_CXX_WARNING_FLAGS
"-Wunused-macros" "-Wunused-member-function" "-Wunused-template"
"-Wused-but-marked-unused" "-Wvector-conversion" "-Wvla"
"-Wweak-template-vtables" "-Wweak-vtables" "-Wzero-as-null-pointer-constant")
# Workaround https://github.com/llvm/llvm-project/issues/43670 - a spurious
# warning with the spaceship operator
set(CLANG_11_CXX_WARNING_FLAGS "-Wno-zero-as-null-pointer-constant")
set(CLANG_LT_13_CXX_WARNING_FLAGS "-Wreserved-id-macro")
set(CLANG_GE_13_CXX_WARNING_FLAGS "-Wreserved-identifier")

Expand Down Expand Up @@ -400,6 +403,8 @@ set(cxx_ge_12 "$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,12.0>")
set(cxx_lt_13 "$<VERSION_LESS:$<CXX_COMPILER_VERSION>,13.0>")
set(cxx_ge_13 "$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,13.0>")
set(cxx_ge_14 "$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,14.0>")
set(cxx_lt_12 "$<VERSION_LESS:$<CXX_COMPILER_VERSION>,12.0>")
set(is_clang11_not_windows "$<AND:${is_clang_not_windows},${cxx_ge_11},${cxx_lt_12}>")
set(is_clang_lt_13_not_windows "$<AND:${is_clang_not_windows},${cxx_lt_13}>")
set(is_clang_ge_13_not_windows "$<AND:${is_clang_not_windows},${cxx_ge_13}>")
set(is_clang_ge_14_not_windows "$<AND:${is_clang_not_windows},${cxx_ge_14}>")
Expand Down Expand Up @@ -705,6 +710,7 @@ function(COMMON_TARGET_PROPERTIES TARGET)
"$<$<AND:${is_standalone},${is_any_msvc}>:${MSVC_CXX_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_clang_cl}>:${MSVC_CLANG_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_any_clang_genex},${is_not_windows}>:${CLANG_CXX_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_clang11_not_windows}>:${CLANG_11_CXX_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_clang_lt_13_not_windows}>:${CLANG_LT_13_CXX_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_clang_ge_13_not_windows}>:${CLANG_GE_13_CXX_WARNING_FLAGS}>"
"$<$<AND:${is_standalone},${is_gxx_genex}>:${GCC_CXX_WARNING_FLAGS}>"
Expand Down
25 changes: 4 additions & 21 deletions qsbr_ptr.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

// IWYU pragma: no_include <__cstddef/byte.h>

#include <compare> // IWYU pragma: keep
#include <cstddef>
#include <iterator>
#include <ranges>
Expand Down Expand Up @@ -257,28 +258,10 @@ class [[nodiscard]] qsbr_ptr : public detail::qsbr_ptr_base {
return get() == other.get();
}

/// Compare less than or equal to \a other.
[[nodiscard, gnu::pure]] constexpr bool operator<=(
/// Three-way compare to \a other.
[[nodiscard, gnu::pure]] constexpr auto operator<=>(
qsbr_ptr other) const noexcept {
return get() <= other.get();
}

/// Compare greater than or equal to \a other.
[[nodiscard, gnu::pure]] constexpr bool operator>=(
qsbr_ptr other) const noexcept {
return get() >= other.get();
}

/// Compare less than \a other.
[[nodiscard, gnu::pure]] constexpr bool operator<(
qsbr_ptr other) const noexcept {
return get() < other.get();
}

/// Compare greater than \a other.
[[nodiscard, gnu::pure]] constexpr bool operator>(
qsbr_ptr other) const noexcept {
return get() > other.get();
return get() <=> other.get();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Get the raw pointer.
Expand Down
1 change: 1 addition & 0 deletions test/test_qsbr_ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <algorithm>
#include <array>
#include <compare> // IWYU pragma: keep
#include <iterator>
#include <span>
#include <utility>
Expand Down
Loading