Copilot commented on code in PR #13269:
URL: https://github.com/apache/trafficserver/pull/13269#discussion_r3437821132
##########
include/cripts/Urls.hpp:
##########
@@ -335,6 +335,25 @@ class Url
using Component::Component;
+ // Shallow copy: segments and `_owner` are shared with the source. The
copy's views
+ // dangle if the source URL's path is rewritten, and Flush()/operator= on
the
+ // copy write back to the source URL via TSUrlPathSet.
+ Path(const self_type &o) : Component(o), _state(o._state ?
std::make_unique<State>(*o._state) : nullptr) {}
Review Comment:
The comment says the copy shares “segments” with the source, but the
container is actually copied via deep-copy of `_state`; only the underlying
`string_view` storage (backing URL buffer) and `_owner` are shared. This is
misleading documentation and makes it harder to reason about lifetime/aliasing.
##########
include/cripts/Urls.hpp:
##########
@@ -485,6 +504,25 @@ class Url
_state->standalone = true;
}
+ // Shallow copy: views and `_owner` are shared with the source. The copy's
views
+ // dangle if the source URL's query is rewritten, and Flush()/operator= on
the
+ // copy write back to the source URL via TSUrlHttpQuerySet.
+ Query(const self_type &o) : Component(o), _state(o._state ?
std::make_unique<State>(*o._state) : nullptr) {}
Review Comment:
The comment says the copy shares “views and `_owner`” (true) but it also
implies the parsed parameter state is shared; however `_state` is deep-copied
here. Clarify that only the underlying URL buffer is shared (via
`string_view`s), while the parsed containers are independent copies.
##########
include/cripts/Urls.hpp:
##########
@@ -335,6 +335,25 @@ class Url
using Component::Component;
+ // Shallow copy: segments and `_owner` are shared with the source. The
copy's views
+ // dangle if the source URL's path is rewritten, and Flush()/operator= on
the
+ // copy write back to the source URL via TSUrlPathSet.
+ Path(const self_type &o) : Component(o), _state(o._state ?
std::make_unique<State>(*o._state) : nullptr) {}
Review Comment:
Defining copy operations for `Url::Path` / `Url::Query` makes `cripts::Url`
implicitly copyable again (including via slicing from derived
`Client::URL`/etc. to `Url`). Copying a `Url` is unsafe because it duplicates
`_bufp/_urlp` and `Reset()` would then release the same TS handle multiple
times. Consider explicitly deleting `Url`'s copy ctor/copy-assign to keep the
wrapper non-copyable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
