Skip to content

Commit

Permalink
clang-tidy: performance-for-range-copy (envoyproxy#7349)
Browse files Browse the repository at this point in the history
Signed-off-by: Derek Argueta <dereka@pinterest.com>
  • Loading branch information
derekargueta authored and mattklein123 committed Jun 21, 2019
1 parent 0ce2190 commit 4b76a2f
Show file tree
Hide file tree
Showing 25 changed files with 45 additions and 43 deletions.
2 changes: 1 addition & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Checks: 'clang-diagnostic-*,clang-analyzer-*,abseil-*,bugprone-*,modernize-*,performance-*,readability-redundant-*,readability-braces-around-statements,readability-container-size-empty'

#TODO(lizan): grow this list, fix possible warnings and make more checks as error
WarningsAsErrors: 'bugprone-assert-side-effect,modernize-make-shared,modernize-make-unique,readability-redundant-smartptr-get,readability-braces-around-statements,readability-redundant-string-cstr,bugprone-use-after-move,readability-container-size-empty,performance-faster-string-find'
WarningsAsErrors: 'bugprone-assert-side-effect,modernize-make-shared,modernize-make-unique,readability-redundant-smartptr-get,readability-braces-around-statements,readability-redundant-string-cstr,bugprone-use-after-move,readability-container-size-empty,performance-faster-string-find,performance-for-range-copy'

CheckOptions:
- key: bugprone-assert-side-effect.AssertMacros
Expand Down
4 changes: 2 additions & 2 deletions source/common/config/filter_json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,14 @@ void FilterJson::translateFaultFilter(
JSON_UTIL_SET_DURATION_FROM_FIELD(*json_config_delay, *delay, fixed_delay, fixed_duration);
}

for (const auto json_header_matcher : json_config.getObjectArray("headers", true)) {
for (const auto& json_header_matcher : json_config.getObjectArray("headers", true)) {
auto* header_matcher = proto_config.mutable_headers()->Add();
RdsJson::translateHeaderMatcher(*json_header_matcher, *header_matcher);
}

JSON_UTIL_SET_STRING(json_config, proto_config, upstream_cluster);

for (auto json_downstream_node : json_config.getStringArray("downstream_nodes", true)) {
for (const auto& json_downstream_node : json_config.getStringArray("downstream_nodes", true)) {
auto* downstream_node = proto_config.mutable_downstream_nodes()->Add();
*downstream_node = json_downstream_node;
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/config/grpc_mux_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ void GrpcMuxImpl::onDiscoveryResponse(
continue;
}
Protobuf::RepeatedPtrField<ProtobufWkt::Any> found_resources;
for (auto watched_resource_name : watch->resources_) {
for (const auto& watched_resource_name : watch->resources_) {
auto it = resources.find(watched_resource_name);
if (it != resources.end()) {
found_resources.Add()->MergeFrom(it->second);
Expand Down Expand Up @@ -205,7 +205,7 @@ void GrpcMuxImpl::onDiscoveryResponse(
void GrpcMuxImpl::onWriteable() { drainRequests(); }

void GrpcMuxImpl::onStreamEstablished() {
for (const auto type_url : subscriptions_) {
for (const auto& type_url : subscriptions_) {
queueDiscoveryRequest(type_url);
}
}
Expand Down
26 changes: 14 additions & 12 deletions source/common/config/rds_json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void RdsJson::translateRateLimit(const Json::Object& json_rate_limit,
JSON_UTIL_SET_INTEGER(json_rate_limit, rate_limit, stage);
JSON_UTIL_SET_STRING(json_rate_limit, rate_limit, disable_key);
const auto actions = json_rate_limit.getObjectArray("actions");
for (const auto json_action : actions) {
for (const auto& json_action : actions) {
auto* action = rate_limit.mutable_actions()->Add();
const std::string type = json_action->getString("type");
if (type == "source_cluster") {
Expand Down Expand Up @@ -119,7 +119,7 @@ void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config,
envoy::api::v2::RouteConfiguration& route_config) {
json_route_config.validateSchema(Json::Schema::ROUTE_CONFIGURATION_SCHEMA);

for (const auto json_virtual_host : json_route_config.getObjectArray("virtual_hosts", true)) {
for (const auto& json_virtual_host : json_route_config.getObjectArray("virtual_hosts", true)) {
auto* virtual_host = route_config.mutable_virtual_hosts()->Add();
translateVirtualHost(*json_virtual_host, *virtual_host);
}
Expand All @@ -129,7 +129,7 @@ void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config,
route_config.add_internal_only_headers(header);
}

for (const auto header_value :
for (const auto& header_value :
json_route_config.getObjectArray("response_headers_to_add", true)) {
auto* header_value_option = route_config.mutable_response_headers_to_add()->Add();
BaseJson::translateHeaderValueOption(*header_value, *header_value_option);
Expand All @@ -140,7 +140,8 @@ void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config,
route_config.add_response_headers_to_remove(header);
}

for (const auto header_value : json_route_config.getObjectArray("request_headers_to_add", true)) {
for (const auto& header_value :
json_route_config.getObjectArray("request_headers_to_add", true)) {
auto* header_value_option = route_config.mutable_request_headers_to_add()->Add();
BaseJson::translateHeaderValueOption(*header_value, *header_value_option);
}
Expand All @@ -159,7 +160,7 @@ void RdsJson::translateVirtualHost(const Json::Object& json_virtual_host,
virtual_host.add_domains(domain);
}

for (const auto json_route : json_virtual_host.getObjectArray("routes", true)) {
for (const auto& json_route : json_virtual_host.getObjectArray("routes", true)) {
auto* route = virtual_host.mutable_routes()->Add();
translateRoute(*json_route, *route);
}
Expand All @@ -169,18 +170,19 @@ void RdsJson::translateVirtualHost(const Json::Object& json_virtual_host,
StringUtil::toUpper(json_virtual_host.getString("require_ssl", "")), &tls_requirement);
virtual_host.set_require_tls(tls_requirement);

for (const auto json_virtual_cluster :
for (const auto& json_virtual_cluster :
json_virtual_host.getObjectArray("virtual_clusters", true)) {
auto* virtual_cluster = virtual_host.mutable_virtual_clusters()->Add();
translateVirtualCluster(*json_virtual_cluster, *virtual_cluster);
}

for (const auto json_rate_limit : json_virtual_host.getObjectArray("rate_limits", true)) {
for (const auto& json_rate_limit : json_virtual_host.getObjectArray("rate_limits", true)) {
auto* rate_limit = virtual_host.mutable_rate_limits()->Add();
translateRateLimit(*json_rate_limit, *rate_limit);
}

for (const auto header_value : json_virtual_host.getObjectArray("request_headers_to_add", true)) {
for (const auto& header_value :
json_virtual_host.getObjectArray("request_headers_to_add", true)) {
auto* header_value_option = virtual_host.mutable_request_headers_to_add()->Add();
BaseJson::translateHeaderValueOption(*header_value, *header_value_option);
}
Expand Down Expand Up @@ -226,12 +228,12 @@ void RdsJson::translateRoute(const Json::Object& json_route, envoy::api::v2::rou
*match->mutable_runtime_fraction());
}

for (const auto json_header_matcher : json_route.getObjectArray("headers", true)) {
for (const auto& json_header_matcher : json_route.getObjectArray("headers", true)) {
auto* header_matcher = match->mutable_headers()->Add();
translateHeaderMatcher(*json_header_matcher, *header_matcher);
}

for (const auto json_query_parameter_matcher :
for (const auto& json_query_parameter_matcher :
json_route.getObjectArray("query_parameters", true)) {
auto* query_parameter_matcher = match->mutable_query_parameters()->Add();
translateQueryParameterMatcher(*json_query_parameter_matcher, *query_parameter_matcher);
Expand Down Expand Up @@ -308,12 +310,12 @@ void RdsJson::translateRoute(const Json::Object& json_route, envoy::api::v2::rou
&priority);
action->set_priority(priority);

for (const auto header_value : json_route.getObjectArray("request_headers_to_add", true)) {
for (const auto& header_value : json_route.getObjectArray("request_headers_to_add", true)) {
auto* header_value_option = route.mutable_request_headers_to_add()->Add();
BaseJson::translateHeaderValueOption(*header_value, *header_value_option);
}

for (const auto json_rate_limit : json_route.getObjectArray("rate_limits", true)) {
for (const auto& json_rate_limit : json_route.getObjectArray("rate_limits", true)) {
auto* rate_limit = action->mutable_rate_limits()->Add();
translateRateLimit(*json_rate_limit, *rate_limit);
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/grpc/google_async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ void GoogleAsyncStreamImpl::metadataTranslate(
Http::HeaderMap& header_map) {
// More painful copying, this time due to the mismatch in header
// representation data structures in Envoy and Google gRPC.
for (auto it : grpc_metadata) {
for (const auto& it : grpc_metadata) {
header_map.addCopy(Http::LowerCaseString(std::string(it.first.data(), it.first.size())),
std::string(it.second.data(), it.second.size()));
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ MessagePtr Utility::prepareHeaders(const ::envoy::api::v2::core::HttpUri& http_u
std::string Utility::queryParamsToString(const QueryParams& params) {
std::string out;
std::string delim = "?";
for (auto p : params) {
for (const auto& p : params) {
absl::StrAppend(&out, delim, p.first, "=", p.second);
delim = "&";
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
cors_policy_ =
std::make_unique<CorsPolicyImpl>(route.route().cors(), factory_context.runtime());
}
for (const auto upgrade_config : route.route().upgrade_configs()) {
for (const auto& upgrade_config : route.route().upgrade_configs()) {
const bool enabled = upgrade_config.has_enabled() ? upgrade_config.enabled().value() : true;
const bool success =
upgrade_map_
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/metadatamatchcriteria_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ MetadataMatchCriteriaImpl::extractMetadataMatchCriteria(const MetadataMatchCrite
}

// Add values from matches, replacing name/values copied from parent.
for (const auto it : matches.fields()) {
for (const auto& it : matches.fields()) {
const auto index_it = existing.find(it.first);
if (index_it != existing.end()) {
v[index_it->second] = std::make_shared<MetadataMatchCriterionImpl>(it.first, it.second);
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/cds_api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ void CdsApiImpl::onConfigUpdate(
exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what()));
}
}
for (auto resource_name : removed_resources) {
for (const auto& resource_name : removed_resources) {
if (cm_.removeCluster(resource_name)) {
any_applied = true;
ENVOY_LOG(debug, "cds: remove cluster '{}'", resource_name);
Expand Down
4 changes: 2 additions & 2 deletions source/common/upstream/load_stats_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void LoadStatsReporter::sendLoadStatsRequest() {
uint64_t rq_error = 0;
uint64_t rq_active = 0;
uint64_t rq_issued = 0;
for (auto host : hosts) {
for (const auto& host : hosts) {
rq_success += host->stats().rq_success_.latch();
rq_error += host->stats().rq_error_.latch();
rq_active += host->stats().rq_active_.value();
Expand Down Expand Up @@ -154,7 +154,7 @@ void LoadStatsReporter::startLoadReportPeriod() {
}
auto& cluster = it->second.get();
for (auto& host_set : cluster.prioritySet().hostSetsPerPriority()) {
for (auto host : host_set->hosts()) {
for (const auto& host : host_set->hosts()) {
host->stats().rq_success_.latch();
host->stats().rq_error_.latch();
host->stats().rq_total_.latch();
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/outlier_detection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ DetectorImpl::DetectorImpl(const Cluster& cluster,
}

DetectorImpl::~DetectorImpl() {
for (auto host : host_monitors_) {
for (const auto& host : host_monitors_) {
if (host.first->healthFlagGet(Host::HealthFlag::FAILED_OUTLIER_CHECK)) {
ASSERT(stats_.ejections_active_.value() > 0);
stats_.ejections_active_.dec();
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/subset_lb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ SubsetLoadBalancer::extractSubsetMetadata(const std::set<std::string>& subset_ke
}

const auto& fields = filter_it->second.fields();
for (const auto key : subset_keys) {
for (const auto& key : subset_keys) {
const auto it = fields.find(key);
if (it == fields.end()) {
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
processFilter(filters[i], i, "http", filter_factories_);
}

for (auto upgrade_config : config.upgrade_configs()) {
for (const auto& upgrade_config : config.upgrade_configs()) {
const std::string& name = upgrade_config.upgrade_type();
const bool enabled = upgrade_config.has_enabled() ? upgrade_config.enabled().value() : true;
if (findUpgradeCaseInsensitive(upgrade_filter_factories_, name) !=
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/transport_sockets/tls/context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c

if (config.certificateValidationContext() != nullptr &&
!config.certificateValidationContext()->verifyCertificateSpkiList().empty()) {
for (auto hash : config.certificateValidationContext()->verifyCertificateSpkiList()) {
for (const auto& hash : config.certificateValidationContext()->verifyCertificateSpkiList()) {
const auto decoded = Base64::decode(hash);
if (decoded.size() != SHA256_DIGEST_LENGTH) {
throw EnvoyException(fmt::format("Invalid base64-encoded SHA-256 {}", hash));
Expand Down
8 changes: 4 additions & 4 deletions source/server/http/admin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ void AdminImpl::writeClustersAsText(Buffer::Instance& response) {
all_stats[gauge->name()] = gauge->value();
}

for (auto stat : all_stats) {
for (const auto& stat : all_stats) {
response.add(fmt::format("{}::{}::{}::{}\n", cluster.second.get().info()->name(),
host->address()->asString(), stat.first, stat.second));
}
Expand Down Expand Up @@ -757,7 +757,7 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::HeaderMap& respo
rc = Http::Code::NotFound;
}
} else { // Display plain stats if format query param is not there.
for (auto stat : all_stats) {
for (const auto& stat : all_stats) {
response.add(fmt::format("{}: {}\n", stat.first, stat.second));
}
// TODO(ramaraochavali): See the comment in ThreadLocalStoreImpl::histograms() for why we use a
Expand All @@ -769,7 +769,7 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::HeaderMap& respo
all_histograms.emplace(histogram->name(), histogram->quantileSummary());
}
}
for (auto histogram : all_histograms) {
for (const auto& histogram : all_histograms) {
response.add(fmt::format("{}: {}\n", histogram.first, histogram.second));
}
}
Expand Down Expand Up @@ -894,7 +894,7 @@ AdminImpl::statsAsJson(const std::map<std::string, uint64_t>& all_stats,
document.SetObject();
rapidjson::Value stats_array(rapidjson::kArrayType);
rapidjson::Document::AllocatorType& allocator = document.GetAllocator();
for (auto stat : all_stats) {
for (const auto& stat : all_stats) {
Value stat_obj;
stat_obj.SetObject();
Value stat_name;
Expand Down
4 changes: 2 additions & 2 deletions test/common/filesystem/directory_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ using EntrySet = std::unordered_set<DirectoryEntry, EntryHash>;
EntrySet getDirectoryContents(const std::string& dir_path, bool recursive) {
Directory directory(dir_path);
EntrySet ret;
for (const DirectoryEntry entry : directory) {
for (const DirectoryEntry& entry : directory) {
ret.insert(entry);
if (entry.type_ == FileType::Directory && entry.name_ != "." && entry.name_ != ".." &&
recursive) {
std::string subdir_name = entry.name_;
EntrySet subdir = getDirectoryContents(dir_path + "/" + subdir_name, recursive);
for (const DirectoryEntry entry : subdir) {
for (const DirectoryEntry& entry : subdir) {
ret.insert({subdir_name + "/" + entry.name_, entry.type_});
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/common/upstream/upstream_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,7 @@ TEST_F(StrictDnsClusterImplTest, LoadAssignmentBasic) {
cluster.prioritySet().hostSetsPerPriority()[0]->healthyHostsPerLocality().get().size());

// Ensure that all host objects in the host list are unique.
for (const auto host : hosts) {
for (const auto& host : hosts) {
EXPECT_EQ(1, std::count(hosts.begin(), hosts.end(), host));
}

Expand Down
2 changes: 1 addition & 1 deletion test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ ConfigHelper::ConfigHelper(const Network::Address::IpVersion version, Api::Api&
}

void ConfigHelper::applyConfigModifiers() {
for (auto config_modifier : config_modifiers_) {
for (const auto& config_modifier : config_modifiers_) {
config_modifier(bootstrap_);
}
config_modifiers_.clear();
Expand Down
2 changes: 1 addition & 1 deletion test/extensions/filters/common/ext_authz/test_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ Response TestCommon::makeAuthzResponse(CheckStatus status, Http::Code status_cod

HeaderValueOptionVector TestCommon::makeHeaderValueOption(KeyValueOptionVector&& headers) {
HeaderValueOptionVector header_option_vector{};
for (auto header : headers) {
for (const auto& header : headers) {
envoy::api::v2::core::HeaderValueOption header_value_option;
auto* mutable_header = header_value_option.mutable_header();
mutable_header->set_key(header.key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class ThriftConnectionManagerTest : public testing::Test {
// Destroy any existing filter first.
filter_ = nullptr;

for (auto counter : store_.counters()) {
for (const auto& counter : store_.counters()) {
counter->reset();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class MetricsServiceIntegrationTest : public Grpc::GrpcClientIntegrationParamTes
const Protobuf::RepeatedPtrField<::io::prometheus::client::MetricFamily>& envoy_metrics =
request_msg.envoy_metrics();

for (::io::prometheus::client::MetricFamily metrics_family : envoy_metrics) {
for (const ::io::prometheus::client::MetricFamily& metrics_family : envoy_metrics) {
if (metrics_family.name() == "cluster.cluster_0.membership_change" &&
metrics_family.type() == ::io::prometheus::client::MetricType::COUNTER) {
known_counter_exists = true;
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http2_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyInvalidMetadata) {
}

void verifyExpectedMetadata(Http::MetadataMap metadata_map, std::set<std::string> keys) {
for (const auto key : keys) {
for (const auto& key : keys) {
// keys are the same as their corresponding values.
EXPECT_EQ(metadata_map.find(key)->second, key);
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http_integration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ void HttpIntegrationTest::waitForNextUpstreamRequest(uint64_t upstream_index) {
}

void HttpIntegrationTest::addFilters(std::vector<std::string> filters) {
for (const auto filter : filters) {
for (const auto& filter : filters) {
config_helper_.addFilter(filter);
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/integration_admin_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class IntegrationAdminTest : public HttpProtocolIntegrationTest {
Json::ObjectSharedPtr statsjson = Json::Factory::loadFromString(stats_json);
EXPECT_TRUE(statsjson->hasObject("stats"));
uint64_t histogram_count = 0;
for (Json::ObjectSharedPtr obj_ptr : statsjson->getObjectArray("stats")) {
for (const Json::ObjectSharedPtr& obj_ptr : statsjson->getObjectArray("stats")) {
if (obj_ptr->hasObject("histograms")) {
histogram_count++;
const Json::ObjectSharedPtr& histograms_ptr = obj_ptr->getObject("histograms");
Expand Down
2 changes: 1 addition & 1 deletion test/server/http/admin_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,7 @@ TEST_P(AdminInstanceTest, RuntimeModifyNoArguments) {

TEST_P(AdminInstanceTest, TracingStatsDisabled) {
const std::string& name = admin_.tracingStats().service_forced_.name();
for (Stats::CounterSharedPtr counter : server_.stats().counters()) {
for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) {
EXPECT_NE(counter->name(), name) << "Unexpected tracing stat found in server stats: " << name;
}
}
Expand Down

0 comments on commit 4b76a2f

Please sign in to comment.