build: opt-in C++20 (MINIO_CPP_STD=20) to unlock span/format/coroutines#237
Conversation
build: opt-in C++20 (MINIO_CPP_STD=20) to unlock span/format/coroutines
📝 WalkthroughWalkthroughThis PR makes the C++ language standard configurable via a cached CMake variable ChangesC++ Standard Opt-in and CI Matrix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)Not applicable — changes are configuration/build-matrix wiring and a small constructor addition, without multi-component runtime interaction flow. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
24-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMatrix duplication across cpp_std pairs.
Each C++20 entry duplicates all fields of its C++17 sibling except
name,os/arch(where applicable) andcpp_std. Given the combos are selective (e.g., no macOS/Windows arm64 CXX20 variant), a full cross-productcpp_std: [17, 20]axis isn't directly applicable, but consider extracting shared fields via YAML anchors or a separateinclude:list to reduce duplication if the matrix grows further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 24 - 91, The CI matrix in the workflow duplicates nearly identical job definitions for each cpp_std variant, making it harder to maintain as it grows. Refactor the strategy used in the matrix section by reusing shared fields from the existing job entries (for example via YAML anchors/aliases or a structured include-style list) so the Ubuntu/macOS/Windows C++17 and C++20 variants stay easy to update while preserving the current selective combinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 145: The CMake invocation in the workflow’s run step is interpolating
matrix values directly into the shell command; switch this step to use env
indirection for the build type and C++ standard instead of embedding `${{
matrix.config.build_type }}` and `${{ matrix.config.cpp_std }}` inline. Update
the workflow job so the `cmake` step reads these values from environment
variables, keeping the command itself static and easier for static analysis to
verify.
In `@CMakeLists.txt`:
- Line 54: The MINIO_CPP_STD cache variable currently accepts any string, which
can let invalid values propagate into cxx_std_${MINIO_CPP_STD} and fail later
with a confusing error. Update the CMakeLists.txt configuration near
set(MINIO_CPP_STD) to validate the value immediately and constrain it to the
supported options only, using a clear fatal message if it is not one of the
allowed standards. Keep the check close to the existing MINIO_CPP_STD definition
so the behavior is easy to find and maintain.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 24-91: The CI matrix in the workflow duplicates nearly identical
job definitions for each cpp_std variant, making it harder to maintain as it
grows. Refactor the strategy used in the matrix section by reusing shared fields
from the existing job entries (for example via YAML anchors/aliases or a
structured include-style list) so the Ubuntu/macOS/Windows C++17 and C++20
variants stay easy to update while preserving the current selective
combinations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c854c9aa-ee35-43be-9802-f56c9481aadc
📒 Files selected for processing (3)
.github/workflows/ci.ymlCMakeLists.txtinclude/miniocpp/types.h
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -a types.h
echo "---- types.h DeleteObject struct ----"
rg -n "struct DeleteObject" -A 30 include/miniocpp/types.h
echo "---- tests.cc usage ----"
fd -a tests.cc
rg -n "DeleteObject" -B2 -A2 tests/tests.ccLength of output: 2687 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---- more context around tests.cc DeleteObject usages ----"
rg -n "DeleteObject" -B5 -A15 tests/tests.cc | head -100
echo "---- CMakeLists.txt cpp std section ----"
rg -n "MINIO_CPP_STD|CXX_STANDARD|Werror|pedantic" CMakeLists.txtLength of output: 2739 🏁 Script executed: #!/bin/bash
set -euo pipefail
which g++ clang++ 2>/dev/null || true
g++ --version 2>/dev/null | head -1 || true
cat > /tmp/repro.cpp << 'EOF'
`#include` <string>
`#include` <list>
struct DeleteObject {
std::string name = {};
std::string version_id = {};
DeleteObject() = default;
explicit DeleteObject(std::string object_name)
: name(std::move(object_name)) {}
~DeleteObject() = default;
};
int main() {
std::list<std::string> objects = {"a", "b"};
std::list<DeleteObject> delete_objects;
for (auto& object : objects) {
delete_objects.push_back(DeleteObject{object});
}
return 0;
}
EOF
g++ -std=c++20 -Wall -Wextra /tmp/repro.cpp -o /tmp/repro 2>&1 || true
echo "---- c++17 ----"
g++ -std=c++17 -Wall -Wextra /tmp/repro.cpp -o /tmp/repro17 2>&1 || trueLength of output: 1012
Here's what's happening: before this PR, The cleanest fix is to update the call site in for (auto& object : objects) {
delete_objects.emplace_back(object);
}
Given this constructor addition is both out of scope for the C++20 opt-in change and is causing this build regression, I'd still recommend either dropping it from this PR (and fixing Let me know if you'd like help wiring up the fix, or if you'd rather split the ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline include/miniocpp/types.h --match DeleteObject --view expanded || true
printf '\n--- include/miniocpp/types.h (lines 340-365) ---\n'
sed -n '340,365p' include/miniocpp/types.h
printf '\n--- DeleteObject usages ---\n'
rg -n -C3 '\bDeleteObject\b|remove_objects|RemoveObjects' include tests .github CMakeLists.txtLength of output: 12433 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n--- tests/tests.cc (lines 150-190) ---\n'
sed -n '150,190p' tests/tests.cc
printf '\n--- tests/tests.cc (lines 556-572) ---\n'
sed -n '556,572p' tests/tests.cc
printf '\n--- Search for likely string->DeleteObject call sites ---\n'
rg -n -C2 'DeleteObject|delete_objects|objects\.push_back|objects\.emplace_back|push_back\(object|push_back\(object_name|=\s*object_name|=\s*object\)' tests/tests.cc includeLength of output: 18909 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n--- Full repo search for DeleteObject usages ---\n'
rg -n -C2 '\bDeleteObject\b|push_back\(|emplace_back\(|objects\s*=|DeleteObject\s*\{' .
printf '\n--- Git diff for PR branch (relevant files) ---\n'
git diff -- include/miniocpp/types.h tests/tests.cc CMakeLists.txt .github/workflows/ci.yml || trueLength of output: 17163
From the current PR snapshot,
That means any existing code doing an implicit conversion, such as: std::list<minio::s3::DeleteObject> objects;
objects.push_back(object_name); // object_name is std::stringwill fail with exactly the kind of diagnostic you pasted. A couple of important notes from the repo state I checked:
So the exact failing call site is not visible in the checked-in snapshot I inspected, but the regression mechanism is clear: adding Two reasonable fixes: // Option 1: keep the convenience behavior
DeleteObject(std::string object_name) : name(std::move(object_name)) {}or, if you want to keep the constructor explicit, update all call sites to use direct construction: objects.emplace_back(object_name);
// or
objects.push_back(minio::s3::DeleteObject{object_name});So I’d revise my earlier assessment slightly: this constructor isn’t just “extra scope” — it also changes public API behavior and can break callers. If the goal of this PR is only the C++17/C++20 build toggle, the safest path is probably to drop this constructor from the PR. If you want to keep it, it should come with call-site updates and a compatibility check. ✏️ Learnings added
|
2 participants
build: opt-in C++20 (MINIO_CPP_STD=20) to unlock span/format/coroutines
fix #222
Summary by CodeRabbit