Copilot commented on code in PR #13269:
URL: https://github.com/apache/trafficserver/pull/13269#discussion_r3416175936
##########
include/cripts/Urls.hpp:
##########
@@ -485,6 +485,23 @@ class Url
_state->standalone = true;
}
+ // Query owns a std::unique_ptr<State>, which makes the implicit copy
operations
+ // deleted
+ 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) {
+ Component::operator=(o);
+ _state = o._state ? std::make_unique<State>(*o._state) : nullptr;
+ }
Review Comment:
`operator=` updates the base `Component` before allocating the new `_state`.
If `std::make_unique<State>(...)` throws, the object is left partially assigned
(base updated, old state retained). Allocate the new state first and then
assign/swap to provide a strong exception guarantee.
##########
src/cripts/tests/query_test.cc:
##########
@@ -31,6 +31,10 @@ do_remap()
{
borrow url = Client::URL::get();
+ // Snapshot the query so we can inspect/mutate without disturbing the live
URL.
+ // Requires Query's explicit copy ctor (Query owns a std::unique_ptr<State>).
+ cripts::Url::Query original_query = url.query;
Review Comment:
The `Query` copy here is still tied to the same underlying `Client::URL` (it
copies the `Component` owner pointer), so it’s not a stable “pre-mutation
snapshot” once `url.query` is changed. Please reword the comment to avoid
implying snapshot semantics.
##########
tests/gold_tests/cripts/files/query_copy.cript:
##########
@@ -0,0 +1,39 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <cripts/Preamble.hpp>
+
+do_send_response()
+{
+ borrow resp = cripts::Client::Response::Get();
+ borrow url = cripts::Client::URL::Get();
+
+ // Exercises Query's explicit copy ctor / copy-assign. The copy must
deep-copy
+ // the unique_ptr<State> so that mutating the copy does not disturb the live
URL.
+ cripts::Url::Query query_copy = url.query;
+ cripts::Url::Query query_assigned;
+
+ query_assigned = url.query;
+ query_copy.Erase("foo");
Review Comment:
This test currently copies `url.query` before the live query is ever parsed,
so `_state` is typically null and the new deep-copy logic isn’t actually
exercised. Force a parse of the live query before copying so the copy ctor /
copy-assign must deep-copy a populated `State` (and the test will catch
accidental sharing).
--
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]
