perf: stop copying every pugixml node().value() into std::string in ParseXML#234
perf: stop copying every pugixml node().value() into std::string in ParseXML#234jiuker wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
String_view migration for Item and ListObjectsResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/baseclient.cc (1)
91-135:⚠️ Potential issue | 🔴 CriticalRedirect handler discards computed error codes and messages due to pass-by-value
std::string_viewparameters.Parameters
codeandmessagearestd::string_viewpassed by value (line 91–92). Inside the function, reassignments likecode = "PermanentRedirect"andmessage = std::move(message_)rebind the local parameters only and do not mutate caller state at line 171 or line 263. Consequently, computed redirect-specific codes (includingRetryHead), region information, and error messages never propagate to callers.Additionally, line 134 (
message = std::move(message_)) attempts an invalid move-assignment ofstd::stringintostd::string_view, which should cause a compilation error.Return an owning struct or use mutable owning out-parameters (e.g.,
std::string&) to restore the output contract.🤖 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 `@src/baseclient.cc` around lines 91 - 135, The HandleRedirectResponse function signature passes code and message as std::string_view by value, causing reassignments within the function to only affect local parameters and not propagate computed values back to the caller. Additionally, attempting to move-assign a std::string into std::string_view at line 134 is invalid. Change the parameters in the HandleRedirectResponse function signature from std::string_view to std::string& (mutable references) for both code and message parameters so that computed redirect codes (like "PermanentRedirect", "Redirect", "BadRequest", "RetryHead") and the constructed message with region information are properly returned to callers.
🤖 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 `@include/miniocpp/response.h`:
- Around line 38-45: The std::string_view fields (code, message, resource,
request_id, host_id, bucket_name, object_name, etag) in the Response struct are
assigned from temporary pointers in Response::ParseXML that become dangling
after the local pugi::xml_document is destroyed. To fix this, either: (1) modify
Response::ParseXML to store the input `data` parameter in the Response.data
field and update all std::string_view member assignments to reference this owned
data instead of the temporary XML document, or (2) revert these std::string_view
fields back to owning std::string types so they maintain their own copies of the
parsed values. The first approach is preferred if zero-copy semantics are
desired; the second provides immediate correctness at the cost of an additional
copy.
---
Outside diff comments:
In `@src/baseclient.cc`:
- Around line 91-135: The HandleRedirectResponse function signature passes code
and message as std::string_view by value, causing reassignments within the
function to only affect local parameters and not propagate computed values back
to the caller. Additionally, attempting to move-assign a std::string into
std::string_view at line 134 is invalid. Change the parameters in the
HandleRedirectResponse function signature from std::string_view to std::string&
(mutable references) for both code and message parameters so that computed
redirect codes (like "PermanentRedirect", "Redirect", "BadRequest", "RetryHead")
and the constructed message with region information are properly returned to
callers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 06643218-d8e3-4b4c-8a37-a559d214c390
📒 Files selected for processing (5)
include/miniocpp/baseclient.hinclude/miniocpp/response.hsrc/baseclient.ccsrc/response.cctests/tests.cc
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)
src/baseclient.cc (1)
230-234:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDangling
std::string_viewfrom temporarystd::stringreturn values.
GetFront()returnsstd::stringby value. Assigning a temporary tostd::string_viewcreates a dangling reference once the temporary is destroyed at the end of the statement.response.request_idandresponse.host_idwill point to freed memory.Apply the same
storelambda pattern used for the 400 case, or store these values inresponse.databefore creating the views.🐛 Proposed fix
response.resource = resource; - response.request_id = response.headers.GetFront("x-amz-request-id"); - response.host_id = response.headers.GetFront("x-amz-id-2"); - response.bucket_name = bucket_name; - response.object_name = object_name; + // Store header values in response.data for stable string_view lifetime + auto store = [&response](const std::string& s) -> std::string_view { + if (s.empty()) return {}; + size_t off = response.data.size(); + response.data.append(s); + return std::string_view(response.data.data() + off, s.size()); + }; + response.request_id = store(response.headers.GetFront("x-amz-request-id")); + response.host_id = store(response.headers.GetFront("x-amz-id-2")); + response.bucket_name = store(bucket_name); + response.object_name = store(object_name);🤖 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 `@src/baseclient.cc` around lines 230 - 234, The assignments to response.request_id and response.host_id create dangling std::string_view references because GetFront() returns temporary std::string objects that are destroyed at the end of the statement. Fix this by applying the store lambda pattern used elsewhere for the 400 case, or alternatively store the string values in response.data first before assigning them to the response.request_id and response.host_id string_view members, ensuring the underlying string data persists beyond the current statement.
🧹 Nitpick comments (3)
include/miniocpp/response.h (2)
56-57: ⚡ Quick winExplicitly delete copy operations since
pugi::xml_documentis not copyable.
pugi::xml_documenthas deleted copy constructor/assignment (it's only movable). Declaring= defaulthere results in these operations being implicitly deleted by the compiler. This works but is confusing—readers may not realize Response is no longer copyable.Using
= deleteexplicitly documents the intent and produces clearer error messages when copy is attempted.📝 Suggested change
- Response(const Response& resp) = default; - Response& operator=(const Response& resp) = default; + Response(const Response& resp) = delete; + Response& operator=(const Response& resp) = delete;🤖 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 `@include/miniocpp/response.h` around lines 56 - 57, The Response class contains a pugi::xml_document member which is not copyable, causing the compiler to implicitly delete the copy constructor and copy assignment operator even though they are declared as default. Replace the `= default` specifiers with `= delete` for both the copy constructor and the copy assignment operator in the Response class to explicitly document that copying is not allowed and provide clearer error messages to users who attempt to copy a Response object.
212-215: 💤 Low valueClarify the move semantics to avoid confusion.
After
Response(std::move(resp)), accessingresp.etagandresp.version_idappears to read from a moved-from object. This is technically safe because only theResponsebase portion was moved—the derived class members remain intact—but the pattern is subtle and error-prone for future maintainers.Consider initializing local references before the move or adding a brief comment explaining why this is safe.
🤖 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 `@include/miniocpp/response.h` around lines 212 - 215, The PutObjectResponse constructor moves the CompleteMultipartUploadResponse parameter to the Response base class but then accesses resp.etag and resp.version_id from the moved-from object, which is confusing even though it is technically safe. To improve clarity for future maintainers, either store the etag and version_id values in local variables before calling Response(std::move(resp)), or add a concise comment explaining that only the Response base portion is moved while the derived class members (etag and version_id) remain valid for access.src/baseclient.cc (1)
1-1: 💤 Low valueFix clang-format style violations.
The pipeline indicates this file is not in Google C/C++ style. Run:
clang-format-20 -i --style=Google src/baseclient.cc🤖 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 `@src/baseclient.cc` at line 1, The file src/baseclient.cc has clang-format style violations and does not conform to Google C/C++ style guide. Run the clang-format command with the Google style configuration to automatically format the entire file and resolve all style violations.Source: Pipeline failures
🤖 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 `@include/miniocpp/client.h`:
- Around line 49-50: The defaulted copy constructor and copy assignment operator
for ListObjectsResult class (at lines 49-50 and lines 85-86) copy the iterator
member itr_ directly, causing it to remain bound to the source object's
container and leading to invalid iterator use. Replace the defaulted copy
constructor and copy assignment operator with custom implementations that first
copy the resp_ container, then rebind itr_ to the corresponding position in the
copied container by calculating the distance from the source iterator to the
source container's begin() and advancing the new iterator by that distance, or
set it to end() if the source iterator is at end().
In `@include/miniocpp/response.h`:
- Around line 287-292: Five copy assignment operators in
include/miniocpp/response.h use the exception-unsafe destructor + placement-new
pattern which leaves the object in an invalid state if the copy constructor
throws. Replace the implementations of Item::operator= (lines 287-292),
ListObjectsResponse::operator= (lines 349-354), DeletedObject::operator= (lines
379-384), DeleteError::operator= (lines 401-406), and
RemoveObjectsResponse::operator= (lines 423-428) with the copy-and-swap idiom.
For each, create a swap member function that exchanges internal state with
another instance, then implement operator= by taking a copy of the parameter,
swapping it with the current object, and returning *this. This provides strong
exception safety by ensuring the object remains valid even if the copy
constructor throws an exception.
---
Outside diff comments:
In `@src/baseclient.cc`:
- Around line 230-234: The assignments to response.request_id and
response.host_id create dangling std::string_view references because GetFront()
returns temporary std::string objects that are destroyed at the end of the
statement. Fix this by applying the store lambda pattern used elsewhere for the
400 case, or alternatively store the string values in response.data first before
assigning them to the response.request_id and response.host_id string_view
members, ensuring the underlying string data persists beyond the current
statement.
---
Nitpick comments:
In `@include/miniocpp/response.h`:
- Around line 56-57: The Response class contains a pugi::xml_document member
which is not copyable, causing the compiler to implicitly delete the copy
constructor and copy assignment operator even though they are declared as
default. Replace the `= default` specifiers with `= delete` for both the copy
constructor and the copy assignment operator in the Response class to explicitly
document that copying is not allowed and provide clearer error messages to users
who attempt to copy a Response object.
- Around line 212-215: The PutObjectResponse constructor moves the
CompleteMultipartUploadResponse parameter to the Response base class but then
accesses resp.etag and resp.version_id from the moved-from object, which is
confusing even though it is technically safe. To improve clarity for future
maintainers, either store the etag and version_id values in local variables
before calling Response(std::move(resp)), or add a concise comment explaining
that only the Response base portion is moved while the derived class members
(etag and version_id) remain valid for access.
In `@src/baseclient.cc`:
- Line 1: The file src/baseclient.cc has clang-format style violations and does
not conform to Google C/C++ style guide. Run the clang-format command with the
Google style configuration to automatically format the entire file and resolve
all style violations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4c96ae8-873f-43c3-9920-525f29af6875
📒 Files selected for processing (5)
include/miniocpp/client.hinclude/miniocpp/response.hsrc/baseclient.ccsrc/client.ccsrc/response.cc
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/baseclient.cc (2)
229-233:⚠️ Potential issue | 🔴 CriticalRemove dangling
std::string_viewreferences at lines 230-231.
response.headers.GetFront()returnsstd::stringby value. Assigning these temporaries directly tostd::string_viewfields (request_idandhost_id) creates dangling references. Either:
- Store the returned
std::stringvalues inresponse.datausing thestorelambda pattern (as done for redirect codes above), or- Assign the
std::stringresults to temporarystd::stringvariables first, then createstd::string_viewpointing into them, or- Change
request_idandhost_idtostd::stringfields instead ofstd::string_view.See lines 1945–1956 for the correct pattern: extract header values into
std::stringvariables before using them.🤖 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 `@src/baseclient.cc` around lines 229 - 233, The assignments to response.request_id and response.host_id are creating dangling std::string_view references because response.headers.GetFront() returns std::string by value, and the temporary is destroyed immediately after assignment. Fix this by either storing the returned std::string values in response.data using the store lambda pattern (as done for redirect codes in the same file), or by assigning the std::string results to temporary std::string variables first before creating std::string_view references to them, or by changing request_id and host_id to be std::string fields instead of std::string_view. Reference the correct pattern shown in the file at lines 1945–1956 for extracting and storing header values properly.
1968-1972:⚠️ Potential issue | 🔴 CriticalDangling
std::string_viewforbucket_nameandobject_name.After moving
responseintoresp, the code assignsargs.bucketandargs.objecttoresp.bucket_nameandresp.object_name, which are inheritedstd::string_viewfields from theResponsebase class. Sinceargsis a function parameter that will be destroyed when the function returns, these views become dangling.These values should be stored in stable storage (e.g.,
resp.data) orStatObjectResponseshould define its ownstd::stringmembers for these fields.🤖 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 `@src/baseclient.cc` around lines 1968 - 1972, The StatObjectResponse constructor assigns args.bucket and args.object (which are parameters that will be destroyed when the function returns) to resp.bucket_name and resp.object_name, which are std::string_view fields inherited from the Response base class. This creates dangling string views. Fix this by either redefining bucket_name and object_name as std::string members directly in StatObjectResponse (instead of inheriting std::string_view versions from the Response base class), or by storing these string values in stable storage within resp.data. The recommended approach is to change the member variable types in StatObjectResponse from std::string_view to std::string for these fields to ensure the strings are owned and managed by the response object itself.
🤖 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.
Outside diff comments:
In `@src/baseclient.cc`:
- Around line 229-233: The assignments to response.request_id and
response.host_id are creating dangling std::string_view references because
response.headers.GetFront() returns std::string by value, and the temporary is
destroyed immediately after assignment. Fix this by either storing the returned
std::string values in response.data using the store lambda pattern (as done for
redirect codes in the same file), or by assigning the std::string results to
temporary std::string variables first before creating std::string_view
references to them, or by changing request_id and host_id to be std::string
fields instead of std::string_view. Reference the correct pattern shown in the
file at lines 1945–1956 for extracting and storing header values properly.
- Around line 1968-1972: The StatObjectResponse constructor assigns args.bucket
and args.object (which are parameters that will be destroyed when the function
returns) to resp.bucket_name and resp.object_name, which are std::string_view
fields inherited from the Response base class. This creates dangling string
views. Fix this by either redefining bucket_name and object_name as std::string
members directly in StatObjectResponse (instead of inheriting std::string_view
versions from the Response base class), or by storing these string values in
stable storage within resp.data. The recommended approach is to change the
member variable types in StatObjectResponse from std::string_view to std::string
for these fields to ensure the strings are owned and managed by the response
object itself.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/miniocpp/response.h (1)
75-88: Macro's defaulted copy operations are implicitly deleted—this is intentional, not a problem.The macro declares
DerivedName(const DerivedName&) = defaultandDerivedName& operator=(const DerivedName&) = default, but sinceResponsebase explicitly deletes copy operations (lines 56-57), these defaulted copies become implicitly deleted per C++ standard. This is not a compilation error or issue: the derived types are never copied in the codebase. All function returns use move semantics or RVO, and no code attempts copy construction or assignment. If the intent is to explicitly disallow copying for clarity, consider removing the defaulted copy operations from the macro or explicitly deleting them instead.🤖 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 `@include/miniocpp/response.h` around lines 75 - 88, The MINIO_S3_DERIVE_FROM_RESPONSE macro currently declares defaulted copy operations (the copy constructor and copy assignment operator) that will be implicitly deleted due to the base Response class explicitly deleting them. Since the derived types are never copied in the codebase and all returns use move semantics, remove the lines declaring DerivedName(const DerivedName&) = default and DerivedName& operator=(const DerivedName&) = default from the macro to make the intent clearer, or alternatively replace them with explicit deletions using = delete to more clearly communicate that copying is not allowed.
🤖 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 `@include/miniocpp/response.h`:
- Around line 213-215: The code accesses etag and version_id members after
moving the Response base class with std::move(resp), which leaves resp in a
moved-from state and results in undefined behavior. In the member initializer
list of this constructor, extract the values of resp.etag and resp.version_id
before calling Response(std::move(resp)) on the base class. You can do this by
using std::exchange to move the members while storing them, or by creating
temporary variables to hold the values before the base class move. Also update
or remove the incorrect comment that claims derived members remain valid after
the base is moved.
---
Nitpick comments:
In `@include/miniocpp/response.h`:
- Around line 75-88: The MINIO_S3_DERIVE_FROM_RESPONSE macro currently declares
defaulted copy operations (the copy constructor and copy assignment operator)
that will be implicitly deleted due to the base Response class explicitly
deleting them. Since the derived types are never copied in the codebase and all
returns use move semantics, remove the lines declaring DerivedName(const
DerivedName&) = default and DerivedName& operator=(const DerivedName&) = default
from the macro to make the intent clearer, or alternatively replace them with
explicit deletions using = delete to more clearly communicate that copying is
not allowed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7786121c-7490-40a1-9a81-ab3e3dbfc729
📒 Files selected for processing (3)
include/miniocpp/client.hinclude/miniocpp/response.hsrc/baseclient.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/baseclient.cc
please check in 327b708 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/miniocpp/response.h (1)
399-400:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCopy constructor discards error details stored in Response base.
The copy constructor initializes
Response()(empty base) and only copiesversion_id, butDeleteErrorinherits error information fromResponse(includingcode,message,object_namevia string_views). Per the review stack context, tests accesserr.object_name, indicating these fields are used.When
DeleteErroris copied, all Response-derived error details are lost. Consider preserving the error information:Suggested approach
If the error details must survive copies, you have two options:
- Store error-relevant fields as owning
std::stringmembers inDeleteErroritself (similar to howItemstoresetag,name, etc.)- Document that
DeleteErrorshould not be copied when Response base fields are populated, and delete the copy constructorGiven that
RemoveObjectsResponse::errorsis astd::list<DeleteError>that may need copying, option 1 is likely required.🤖 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 `@include/miniocpp/response.h` around lines 399 - 400, The copy constructor for DeleteError in include/miniocpp/response.h at lines 399-400 initializes the Response base class with an empty constructor instead of properly copying it. This causes error information (code, message, object_name) stored in the Response base to be lost during copy operations. Since DeleteError is stored in std::list<DeleteError> in RemoveObjectsResponse which requires copying, and tests access err.object_name, you must preserve this error information. Either modify the copy constructor to properly initialize the Response base class by passing the other object's data to it, or add owning std::string member variables to DeleteError (similar to how Item stores etag and name) to store the error details directly and copy those in the constructor. Ensure that after copying a DeleteError, all error details remain accessible.
🤖 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.
Outside diff comments:
In `@include/miniocpp/response.h`:
- Around line 399-400: The copy constructor for DeleteError in
include/miniocpp/response.h at lines 399-400 initializes the Response base class
with an empty constructor instead of properly copying it. This causes error
information (code, message, object_name) stored in the Response base to be lost
during copy operations. Since DeleteError is stored in std::list<DeleteError> in
RemoveObjectsResponse which requires copying, and tests access err.object_name,
you must preserve this error information. Either modify the copy constructor to
properly initialize the Response base class by passing the other object's data
to it, or add owning std::string member variables to DeleteError (similar to how
Item stores etag and name) to store the error details directly and copy those in
the constructor. Ensure that after copying a DeleteError, all error details
remain accessible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f44e03e1-a942-4a2a-bd9f-7efecc9fbecd
📒 Files selected for processing (1)
include/miniocpp/response.h
jiuker
force-pushed
the
fix-issue-221
branch
5 times, most recently
from
9144d8a to
ed0a426
Compare
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)
include/miniocpp/response.h (1)
241-280:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix copy/lifetime semantics for view-backed list results.
ItemandListObjectsResponseremain copyable, but their views can point intoListObjectsResponse::owned_. A default copy duplicatesowned_while leaving existingstd::string_views pointing at the source response’s list; copiedItems carry no backing owner at all. When the source response/result is replaced or destroyed, those copied views dangle.This can surface in pagination/copy paths such as
ListObjectsResult::operator++(int), which copies the result before repopulatingresp_. Either make the backing storage shared across copies, add custom copy operations that rebase all views, or make these types move-only and update downstream copy sites.🤖 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 `@include/miniocpp/response.h` around lines 241 - 280, The `Item` and `ListObjectsResponse` structs contain multiple `std::string_view` members that reference memory owned by `ListObjectsResponse::owned_` and `ListObjectsResponse::doc_`. When these objects are copied using default copy semantics, the views are shallow-copied while the backing storage remains in the original object, causing the copied views to dangle when the source is destroyed. Fix this by either: (1) making `Item` and `ListObjectsResponse` move-only by deleting their copy constructors and copy assignment operators, then updating all downstream code that copies these types (such as in `ListObjectsResult::operator++(int)`); (2) implementing custom copy constructors and copy assignment operators for both `Item` and `ListObjectsResponse` that properly rebase all string_view members to point into the destination's `owned_` and `doc_` storage; or (3) converting `owned_` and `doc_` to shared pointers that are retained across copies, ensuring views remain valid throughout the object's lifetime.
🤖 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 `@include/miniocpp/response.h`:
- Around line 21-25: The file response.h is missing direct includes for types it
declares publicly. Add two standard library headers to the existing include
block: include the memory header for std::shared_ptr (used at line 278) and
include the string_view header for std::string_view (used throughout the file at
lines 70, 136, 174, 243-250, 256, 269-272, 283-296, 304, 345, 383, 402, 450,
467, 485, 520). These should be added to the include section between lines 21-25
to ensure the header is self-contained and does not rely on transitive includes.
In `@src/response.cc`:
- Around line 163-164: The ParseXML function receives a std::string_view
parameter but calls load_string(data.data()) which only passes the pointer and
treats the input as NUL-terminated, ignoring the view's size information. This
causes issues with non-NUL-terminated buffers and embedded NUL bytes. Replace
the load_string call with load_buffer(data.data(), data.size()) to properly
respect the buffer's actual size and prevent reading past the bounds or
truncating on embedded NUL bytes.
---
Outside diff comments:
In `@include/miniocpp/response.h`:
- Around line 241-280: The `Item` and `ListObjectsResponse` structs contain
multiple `std::string_view` members that reference memory owned by
`ListObjectsResponse::owned_` and `ListObjectsResponse::doc_`. When these
objects are copied using default copy semantics, the views are shallow-copied
while the backing storage remains in the original object, causing the copied
views to dangle when the source is destroyed. Fix this by either: (1) making
`Item` and `ListObjectsResponse` move-only by deleting their copy constructors
and copy assignment operators, then updating all downstream code that copies
these types (such as in `ListObjectsResult::operator++(int)`); (2) implementing
custom copy constructors and copy assignment operators for both `Item` and
`ListObjectsResponse` that properly rebase all string_view members to point into
the destination's `owned_` and `doc_` storage; or (3) converting `owned_` and
`doc_` to shared pointers that are retained across copies, ensuring views remain
valid throughout the object's lifetime.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65594e4b-2ef9-40fe-86cd-d02a6d5fd325
📒 Files selected for processing (2)
include/miniocpp/response.hsrc/response.cc
1 participant
perf: stop copying every pugixml node().value() into std::string in ParseXML
fix #221
Summary by CodeRabbit