Copilot commented on code in PR #13269:
URL: https://github.com/apache/trafficserver/pull/13269#discussion_r3438216693
##########
include/cripts/Urls.hpp:
##########
@@ -335,6 +335,25 @@ class Url
using Component::Component;
+ // _state is deep-copied, but the segment `string_view`s (and
Component::_owner) still
+ // reference the source URL. 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) {}
+
+ self_type &
+ operator=(const self_type &o)
+ {
+ if (this != &o) {
+ auto new_state = o._state ? std::make_unique<State>(*o._state) :
nullptr;
+ Component::operator=(o);
+ _state = std::move(new_state);
+ }
+ return *this;
+ }
Review Comment:
`std::move` is used here, but this header does not include `<utility>`.
Relying on transitive includes can break builds with different standard library
implementations. A local fix is to avoid `std::move` by swapping the
unique_ptrs instead (or add `<utility>` at the top of the file).
##########
include/cripts/Urls.hpp:
##########
@@ -485,6 +504,25 @@ class Url
_state->standalone = true;
}
+ // _state is deep-copied, but the parameter `string_view`s (and
Component::_owner) still
+ // reference the source URL. 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) {}
+
+ self_type &
+ operator=(const self_type &o)
+ {
+ if (this != &o) {
+ auto new_state = o._state ? std::make_unique<State>(*o._state) :
nullptr;
+ Component::operator=(o);
+ _state = std::move(new_state);
+ }
+ return *this;
+ }
Review Comment:
`std::move` is used here, but this header does not include `<utility>`. To
keep the header self-contained, avoid relying on transitive includes by
swapping the unique_ptrs instead (or add `<utility>` at the top of the file).
##########
include/cripts/Urls.hpp:
##########
@@ -485,6 +504,25 @@ class Url
_state->standalone = true;
}
+ // _state is deep-copied, but the parameter `string_view`s (and
Component::_owner) still
+ // reference the source URL. 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:
Now that this class has a copy-assignment operator, the comment’s
`operator=` reference is ambiguous (it could read as copy-assign). This is
specifically about `operator=(cripts::string_view)` (and `Flush()` calling it).
Clarifying avoids confusion for future readers.
##########
include/cripts/Urls.hpp:
##########
@@ -335,6 +335,25 @@ class Url
using Component::Component;
+ // _state is deep-copied, but the segment `string_view`s (and
Component::_owner) still
+ // reference the source URL. 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:
Now that this class has a copy-assignment operator, the comment’s
`operator=` reference is ambiguous (it could read as copy-assign). This is
specifically about `operator=(cripts::string_view)` (and `Flush()` calling it).
Clarifying avoids confusion for future readers.
--
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]
