From 2b1b9f9c1efb6f2f6a53c3f60c61304ddb7ecc19 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 26 Aug 2026 17:49:41 -0700 Subject: [PATCH 1/2] Remove unnecessary copies flagged by static analysis Add std::move where a local's last use was a copy into a container or a by-value parameter, and bind a few config-parse locals by const reference where the expression already returns a reference into longer-lived storage. Each site was checked individually: the moved-from objects are never read again, and the const-reference binds point into yaml-cpp node storage owned by the document's shared memory holder rather than into the temporary Node handle, so they cannot dangle. Findings in test helpers and a few sites where the copy was either required or not actually a copy were left alone. --- plugins/cachekey/pattern.cc | 8 ++++---- plugins/experimental/rate_limit/limiter.h | 2 +- plugins/header_rewrite/parser.cc | 10 +++++----- src/proxy/http/remap/NextHopConsistentHash.cc | 4 ++-- src/proxy/http/remap/NextHopSelectionStrategy.cc | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/plugins/cachekey/pattern.cc b/plugins/cachekey/pattern.cc index 515cd8f4f0b..22453bfaff4 100644 --- a/plugins/cachekey/pattern.cc +++ b/plugins/cachekey/pattern.cc @@ -145,7 +145,7 @@ Pattern::process(const String &subject, StringVector &result) /* Replacement pattern was provided in the configuration - capture and replace. */ String element; if (replace(subject, element)) { - result.push_back(element); + result.push_back(std::move(element)); } else { return false; } @@ -154,11 +154,11 @@ Pattern::process(const String &subject, StringVector &result) StringVector captures; if (capture(subject, captures)) { if (captures.size() == 1) { - result.push_back(captures[0]); + result.push_back(std::move(captures[0])); } else { StringVector::iterator it = captures.begin() + 1; for (; it != captures.end(); it++) { - result.push_back(*it); + result.push_back(std::move(*it)); } } } else { @@ -224,7 +224,7 @@ Pattern::capture(const String &subject, StringVector &result) String dst(capture.data(), capture.length()); CacheKeyDebug("capturing '%s' %d", dst.c_str(), i); - result.push_back(dst); + result.push_back(std::move(dst)); } return true; diff --git a/plugins/experimental/rate_limit/limiter.h b/plugins/experimental/rate_limit/limiter.h index 038c295faf1..cbddb6859a8 100644 --- a/plugins/experimental/rate_limit/limiter.h +++ b/plugins/experimental/rate_limit/limiter.h @@ -225,7 +225,7 @@ template class RateLimiter std::string tag = metrics["tag"] ? metrics["tag"].as() : name(); Dbg(dbg_ctl, "Metrics for selector rule: %s(%s, %s)", name().c_str(), prefix.c_str(), tag.c_str()); - initializeMetrics(RATE_LIMITER_TYPE_SNI, prefix, tag); + initializeMetrics(RATE_LIMITER_TYPE_SNI, std::move(prefix), std::move(tag)); } return true; diff --git a/plugins/header_rewrite/parser.cc b/plugins/header_rewrite/parser.cc index c1f467d39a8..ba41e644cc7 100644 --- a/plugins/header_rewrite/parser.cc +++ b/plugins/header_rewrite/parser.cc @@ -185,11 +185,11 @@ Parser::preprocess(std::vector tokens) // This produces an error, but it's not fatal for load / reload. ToDo: ATS v11 fix. TSError("[%s] Duplicate modifier: %s", PLUGIN_NAME, t.c_str()); } else { - _mods.push_back(t); + _mods.push_back(std::move(t)); } } } else { - _mods.push_back(m); + _mods.push_back(std::move(m)); } tokens.pop_back(); // consume it, so we don't concatenate it into the value } else { @@ -228,7 +228,7 @@ Parser::preprocess(std::vector tokens) _arg = tokens[1] + tokens[2]; } else if (tokens.size() > 1) { // This is for the regular expression, which for some reason has its own handling?? ToDo: Why ? - _arg = tokens[1]; + _arg = std::move(tokens[1]); } else { // This would be for hook conditions, which has no argument. _arg = ""; @@ -240,9 +240,9 @@ Parser::preprocess(std::vector tokens) } } else { // Operator has no qualifiers, but could take an optional second argument - _op = tokens[0]; + _op = std::move(tokens[0]); if (tokens.size() > 1) { - _arg = tokens[1]; + _arg = std::move(tokens[1]); if (tokens.size() > 2) { for (auto it = tokens.begin() + 2; it != tokens.end(); it++) { diff --git a/src/proxy/http/remap/NextHopConsistentHash.cc b/src/proxy/http/remap/NextHopConsistentHash.cc index e307844fd3b..31faae7cf74 100644 --- a/src/proxy/http/remap/NextHopConsistentHash.cc +++ b/src/proxy/http/remap/NextHopConsistentHash.cc @@ -96,7 +96,7 @@ NextHopConsistentHash::NextHopConsistentHash(const std::string_view name, const try { if (n["hash_url"]) { - auto hash_url_val = n["hash_url"].Scalar(); + auto const &hash_url_val = n["hash_url"].Scalar(); if (hash_url_val == hash_url_request) { hash_url = NHHashUrlType::REQUEST; } else if (hash_url_val == hash_url_cache) { @@ -116,7 +116,7 @@ NextHopConsistentHash::NextHopConsistentHash(const std::string_view name, const try { if (n["hash_key"]) { - auto hash_key_val = n["hash_key"].Scalar(); + auto const &hash_key_val = n["hash_key"].Scalar(); if (hash_key_val == hash_key_url) { hash_key = NHHashKeyType::URL_HASH_KEY; } else if (hash_key_val == hash_key_hostname) { diff --git a/src/proxy/http/remap/NextHopSelectionStrategy.cc b/src/proxy/http/remap/NextHopSelectionStrategy.cc index 6ca4cf78ef6..76593452e2b 100644 --- a/src/proxy/http/remap/NextHopSelectionStrategy.cc +++ b/src/proxy/http/remap/NextHopSelectionStrategy.cc @@ -55,7 +55,7 @@ NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, try { // scheme is optional, and strategies with no scheme will match hosts with no scheme if (n["scheme"]) { - auto scheme_val = n["scheme"].Scalar(); + auto const &scheme_val = n["scheme"].Scalar(); if (scheme_val == "http") { scheme = NHSchemeType::HTTP; } else if (scheme_val == "https") { @@ -97,7 +97,7 @@ NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, if (failover_node_n) { ts::Yaml::Map failover_node{failover_node_n}; if (failover_node["ring_mode"]) { - auto ring_mode_val = failover_node["ring_mode"].Scalar(); + auto const &ring_mode_val = failover_node["ring_mode"].Scalar(); if (ring_mode_val == alternate_rings) { ring_mode = NHRingMode::ALTERNATE_RING; } else if (ring_mode_val == exhaust_rings) { @@ -389,7 +389,7 @@ template <> struct convert { // scheme is optional, and strategies with no scheme will match hosts with no scheme if (map["scheme"]) { - const auto scheme_val = map["scheme"].Scalar(); + const auto &scheme_val = map["scheme"].Scalar(); if (scheme_val == "http") { nh.scheme = NHSchemeType::HTTP; } else if (scheme_val == "https") { From 1f50277f870560b5a5488f358c5e36efe235877f Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 27 Aug 2026 11:14:03 -0700 Subject: [PATCH 2/2] Drop the const-reference binds GCC rejects GCC's -Wdangling-reference cannot prove that the reference returned by YAML::Node::Scalar() does not point into the temporary Node returned by operator[], and the tree builds with -Werror. The copies these avoided are small and the analysis findings behind them are not worth a suppression, so keep the by-value locals. --- src/proxy/http/remap/NextHopConsistentHash.cc | 4 ++-- src/proxy/http/remap/NextHopSelectionStrategy.cc | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/proxy/http/remap/NextHopConsistentHash.cc b/src/proxy/http/remap/NextHopConsistentHash.cc index 31faae7cf74..e307844fd3b 100644 --- a/src/proxy/http/remap/NextHopConsistentHash.cc +++ b/src/proxy/http/remap/NextHopConsistentHash.cc @@ -96,7 +96,7 @@ NextHopConsistentHash::NextHopConsistentHash(const std::string_view name, const try { if (n["hash_url"]) { - auto const &hash_url_val = n["hash_url"].Scalar(); + auto hash_url_val = n["hash_url"].Scalar(); if (hash_url_val == hash_url_request) { hash_url = NHHashUrlType::REQUEST; } else if (hash_url_val == hash_url_cache) { @@ -116,7 +116,7 @@ NextHopConsistentHash::NextHopConsistentHash(const std::string_view name, const try { if (n["hash_key"]) { - auto const &hash_key_val = n["hash_key"].Scalar(); + auto hash_key_val = n["hash_key"].Scalar(); if (hash_key_val == hash_key_url) { hash_key = NHHashKeyType::URL_HASH_KEY; } else if (hash_key_val == hash_key_hostname) { diff --git a/src/proxy/http/remap/NextHopSelectionStrategy.cc b/src/proxy/http/remap/NextHopSelectionStrategy.cc index 76593452e2b..6ca4cf78ef6 100644 --- a/src/proxy/http/remap/NextHopSelectionStrategy.cc +++ b/src/proxy/http/remap/NextHopSelectionStrategy.cc @@ -55,7 +55,7 @@ NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, try { // scheme is optional, and strategies with no scheme will match hosts with no scheme if (n["scheme"]) { - auto const &scheme_val = n["scheme"].Scalar(); + auto scheme_val = n["scheme"].Scalar(); if (scheme_val == "http") { scheme = NHSchemeType::HTTP; } else if (scheme_val == "https") { @@ -97,7 +97,7 @@ NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, if (failover_node_n) { ts::Yaml::Map failover_node{failover_node_n}; if (failover_node["ring_mode"]) { - auto const &ring_mode_val = failover_node["ring_mode"].Scalar(); + auto ring_mode_val = failover_node["ring_mode"].Scalar(); if (ring_mode_val == alternate_rings) { ring_mode = NHRingMode::ALTERNATE_RING; } else if (ring_mode_val == exhaust_rings) { @@ -389,7 +389,7 @@ template <> struct convert { // scheme is optional, and strategies with no scheme will match hosts with no scheme if (map["scheme"]) { - const auto &scheme_val = map["scheme"].Scalar(); + const auto scheme_val = map["scheme"].Scalar(); if (scheme_val == "http") { nh.scheme = NHSchemeType::HTTP; } else if (scheme_val == "https") {