From 877d1852e0e41fbc313df178aef187070463775a Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Wed, 29 Jul 2020 16:50:05 +0000 Subject: [PATCH 1/7] Subject: replacing .first/.second with meaningful names Signed-off-by: Yifan Yang --- include/envoy/registry/registry.h | 42 +++++++++---------- .../access_log/access_log_manager_impl.cc | 10 ++--- source/common/common/utility.cc | 10 ++--- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index ef12fff187b6c..ad5243c5ae3f6 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -164,9 +164,9 @@ template class FactoryRegistry : public Logger::Loggable class FactoryRegistry : public Logger::Loggable class FactoryRegistry : public Logger::Loggable> buildFactoriesByType() { auto mapping = std::make_unique>(); - for (const auto& factory : factories()) { - if (factory.second == nullptr) { + for (const auto& [factory_name, factory] : factories()) { + if (factory == nullptr) { continue; } // Skip untyped factories. - std::string config_type = factory.second->configType(); + std::string config_type = factory->configType(); if (config_type.empty()) { continue; } @@ -356,14 +356,14 @@ template class FactoryRegistry : public Logger::Loggablefind(config_type); - if (it != mapping->end() && it->second != factory.second) { + if (it != mapping->end() && it->second != factory) { // Mark double-registered types with a nullptr. // See issue https://github.com/envoyproxy/envoy/issues/9643. ENVOY_LOG(warn, "Double registration for type: '{}' by '{}' and '{}'", config_type, - factory.second->name(), it->second ? it->second->name() : ""); + factory->name(), it->second ? it->second->name() : ""); it->second = nullptr; } else { - mapping->emplace(std::make_pair(config_type, factory.second)); + mapping->emplace(std::make_pair(config_type, factory)); } const Protobuf::Descriptor* previous = @@ -464,21 +464,21 @@ template class FactoryRegistry : public Logger::Loggablename(), prev_by_name->configType()); } - for (auto mapping : prev_deprecated_names) { - deprecatedFactoryNames().erase(mapping.first); + for (auto [deprecated_name, restored_name] : prev_deprecated_names) { + deprecatedFactoryNames().erase(deprecated_name); - ENVOY_LOG(info, "Removed deprecated name '{}'", mapping.first); + ENVOY_LOG(info, "Removed deprecated name '{}'", deprecated_name); - if (!mapping.second.empty()) { - deprecatedFactoryNames().emplace(std::make_pair(mapping.first, mapping.second)); + if (!restored_name.empty()) { + deprecatedFactoryNames().emplace(std::make_pair(deprecated_name, restored_name)); - auto* deprecated_factory = getFactory(mapping.second); + auto* deprecated_factory = getFactory(restored_name); RELEASE_ASSERT(deprecated_factory != nullptr, "failed to restore deprecated factory name"); - factories().emplace(mapping.second, deprecated_factory); + factories().emplace(restored_name, deprecated_factory); - ENVOY_LOG(info, "Restored deprecated name '{}' (mapped to '{}'", mapping.first, - mapping.second); + ENVOY_LOG(info, "Restored deprecated name '{}' (mapped to '{}'", deprecated_name, + restored_name); } } diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 055e602bdcfb5..4393fe94c0b8f 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -12,16 +12,16 @@ namespace Envoy { namespace AccessLog { AccessLogManagerImpl::~AccessLogManagerImpl() { - for (auto& access_log : access_logs_) { - ENVOY_LOG(debug, "destroying access logger {}", access_log.first); - access_log.second.reset(); + for (auto& [log_key, log_file_ptr] : access_logs_) { + ENVOY_LOG(debug, "destroying access logger {}", log_key); + log_file_ptr.reset(); } ENVOY_LOG(debug, "destroyed access loggers"); } void AccessLogManagerImpl::reopen() { - for (auto& access_log : access_logs_) { - access_log.second->reopen(); + for (auto& [log_key, log_file_ptr] : access_logs_) { + log_file_ptr->reopen(); } } diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 2017bbcdfa669..1d7a5005129a4 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -507,12 +507,12 @@ std::string StringUtil::removeCharacters(const absl::string_view& str, const auto intervals = remove_characters.toVector(); std::vector pieces; pieces.reserve(intervals.size()); - for (const auto& interval : intervals) { - if (interval.first != pos) { - ASSERT(interval.second <= str.size()); - pieces.push_back(str.substr(pos, interval.first - pos)); + for (const auto& [left_bound, right_bound] : intervals) { + if (left_bound != pos) { + ASSERT(right_bound <= str.size()); + pieces.push_back(str.substr(pos, left_bound - pos)); } - pos = interval.second; + pos = right_bound; } if (pos != str.size()) { pieces.push_back(str.substr(pos)); From aa1030c6da8f3c487839919cd10b7216fd39e205 Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Wed, 29 Jul 2020 16:54:18 +0000 Subject: [PATCH 2/7] changed a naming Signed-off-by: Yifan Yang --- include/envoy/registry/registry.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index ad5243c5ae3f6..432a661c05ff0 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -464,21 +464,21 @@ template class FactoryRegistry : public Logger::Loggablename(), prev_by_name->configType()); } - for (auto [deprecated_name, restored_name] : prev_deprecated_names) { + for (auto [prev_deprecated_name, instead_name] : prev_deprecated_names) { deprecatedFactoryNames().erase(deprecated_name); ENVOY_LOG(info, "Removed deprecated name '{}'", deprecated_name); - if (!restored_name.empty()) { - deprecatedFactoryNames().emplace(std::make_pair(deprecated_name, restored_name)); + if (!instead_name.empty()) { + deprecatedFactoryNames().emplace(std::make_pair(deprecated_name, instead_name)); - auto* deprecated_factory = getFactory(restored_name); + auto* deprecated_factory = getFactory(instead_name); RELEASE_ASSERT(deprecated_factory != nullptr, "failed to restore deprecated factory name"); - factories().emplace(restored_name, deprecated_factory); + factories().emplace(instead_name, deprecated_factory); ENVOY_LOG(info, "Restored deprecated name '{}' (mapped to '{}'", deprecated_name, - restored_name); + instead_name); } } From 74f22a9b77ae777084128373f2b8aad3cfaa6d2b Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Wed, 29 Jul 2020 17:02:45 +0000 Subject: [PATCH 3/7] more name changes Signed-off-by: Yifan Yang --- include/envoy/registry/registry.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 432a661c05ff0..4ed1c2e646741 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -465,19 +465,19 @@ template class FactoryRegistry : public Logger::Loggable Date: Wed, 29 Jul 2020 18:43:34 +0000 Subject: [PATCH 4/7] change some other names Signed-off-by: Yifan Yang --- include/envoy/registry/registry.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 4ed1c2e646741..2b85df27c2e93 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -261,9 +261,9 @@ template class FactoryRegistry : public Logger::Loggable class FactoryRegistry : public Logger::Loggablename(), prev_by_name->configType()); } - for (auto [prev_deprecated_name, instead_name] : prev_deprecated_names) { + for (auto [prev_deprecated_name, mapped_canonical_name] : prev_deprecated_names) { deprecatedFactoryNames().erase(prev_deprecated_name); ENVOY_LOG(info, "Removed deprecated name '{}'", prev_deprecated_name); - if (!instead_name.empty()) { - deprecatedFactoryNames().emplace(std::make_pair(prev_deprecated_name, instead_name)); + if (!mapped_canonical_name.empty()) { + deprecatedFactoryNames().emplace( + std::make_pair(prev_deprecated_name, mapped_canonical_name)); - auto* deprecated_factory = getFactory(instead_name); + auto* deprecated_factory = getFactory(mapped_canonical_name); RELEASE_ASSERT(deprecated_factory != nullptr, "failed to restore deprecated factory name"); - factories().emplace(instead_name, deprecated_factory); + factories().emplace(mapped_canonical_name, deprecated_factory); ENVOY_LOG(info, "Restored deprecated name '{}' (mapped to '{}'", prev_deprecated_name, - instead_name); + mapped_canonical_name); } } From 5a21772aea287cc1c40b552903dfdff2efb41f0d Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Wed, 29 Jul 2020 21:32:28 +0000 Subject: [PATCH 5/7] cleanup: replacing .first/.second with meaningful names in common/config Signed-off-by: Yifan Yang --- source/common/config/delta_subscription_state.cc | 11 +++++------ source/common/config/metadata.h | 7 ++++--- source/common/config/new_grpc_mux_impl.cc | 11 +++++------ source/common/config/type_to_endpoint.cc | 11 ++++------- source/common/config/watch_map.cc | 16 +++++++--------- 5 files changed, 25 insertions(+), 31 deletions(-) diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 2763fdfd9dff1..0ca9bc3bfe8ec 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -1,9 +1,8 @@ -#include "common/config/delta_subscription_state.h" - #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/assert.h" #include "common/common/hash.h" +#include "common/config/delta_subscription_state.h" #include "common/config/utility.h" namespace Envoy { @@ -124,16 +123,16 @@ DeltaSubscriptionState::getNextRequestAckless() { // initial_resource_versions "must be populated for first request in a stream". // Also, since this might be a new server, we must explicitly state *all* of our subscription // interest. - for (auto const& resource : resource_versions_) { + for (auto const& [resource_name, resource_version] : resource_versions_) { // Populate initial_resource_versions with the resource versions we currently have. // Resources we are interested in, but are still waiting to get any version of from the // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) - if (!resource.second.waitingForServer()) { - (*request.mutable_initial_resource_versions())[resource.first] = resource.second.version(); + if (!resource_version.waitingForServer()) { + (*request.mutable_initial_resource_versions())[resource_name] = resource_version.version(); } // As mentioned above, fill resource_names_subscribe with everything, including names we // have yet to receive any resource for. - names_added_.insert(resource.first); + names_added_.insert(resource_name); } names_removed_.clear(); } diff --git a/source/common/config/metadata.h b/source/common/config/metadata.h index ed4fcd96c2703..efac4eff7e59d 100644 --- a/source/common/config/metadata.h +++ b/source/common/config/metadata.h @@ -116,10 +116,11 @@ template class TypedMetadataImpl : public TypedMetadata */ void populateFrom(const envoy::config::core::v3::Metadata& metadata) { auto& data_by_key = metadata.filter_metadata(); - for (const auto& it : Registry::FactoryRegistry::factories()) { - const auto& meta_iter = data_by_key.find(it.first); + for (const auto& [factory_name, factory] : + Registry::FactoryRegistry::factories()) { + const auto& meta_iter = data_by_key.find(factory_name); if (meta_iter != data_by_key.end()) { - data_[it.second->name()] = it.second->parse(meta_iter->second); + data_[factory->name()] = factory->parse(meta_iter->second); } } } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index c7caaf04f664b..d9934ff57da51 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -1,10 +1,9 @@ -#include "common/config/new_grpc_mux_impl.h" - #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/assert.h" #include "common/common/backoff_strategy.h" #include "common/common/token_bucket_impl.h" +#include "common/config/new_grpc_mux_impl.h" #include "common/config/utility.h" #include "common/config/version_converter.h" #include "common/memory/utils.h" @@ -73,8 +72,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( } void NewGrpcMuxImpl::onStreamEstablished() { - for (auto& sub : subscriptions_) { - sub.second->sub_state_.markStreamFresh(); + for (auto& [type_url, sub] : subscriptions_) { + sub->sub_state_.markStreamFresh(); } trySendDiscoveryRequests(); } @@ -88,8 +87,8 @@ void NewGrpcMuxImpl::onEstablishmentFailure() { absl::flat_hash_map all_subscribed; absl::flat_hash_map already_called; do { - for (auto& sub : subscriptions_) { - all_subscribed[sub.first] = &sub.second->sub_state_; + for (auto& [type_url, sub] : subscriptions_) { + all_subscribed[type_url] = &sub->sub_state_; } for (auto& sub : all_subscribed) { if (already_called.insert(sub).second) { // insert succeeded ==> not already called diff --git a/source/common/config/type_to_endpoint.cc b/source/common/config/type_to_endpoint.cc index 9821b288dcbca..697b52b72721d 100644 --- a/source/common/config/type_to_endpoint.cc +++ b/source/common/config/type_to_endpoint.cc @@ -1,7 +1,6 @@ -#include "common/config/type_to_endpoint.h" - #include "envoy/annotations/resource.pb.h" +#include "common/config/type_to_endpoint.h" #include "common/grpc/common.h" // API_NO_BOOST_FILE @@ -185,12 +184,11 @@ TypeUrlToVersionedServiceMap* buildTypeUrlToServiceMap() { }}, }}, }) { - for (const auto& registered_service : registered) { - const TypeUrl resource_type_url = getResourceTypeUrl(registered_service.first); + for (const auto& [registered_service_name, registered_service_info] : registered) { + const TypeUrl resource_type_url = getResourceTypeUrl(registered_service_name); VersionedService& service = (*type_url_to_versioned_service_map)[resource_type_url]; - for (const auto& versioned_service_name : registered_service.second.names_) { - const ServiceName& service_name = versioned_service_name.second; + for (const auto& [transport_api_version, service_name] : registered_service_info.names_) { const auto* service_desc = Protobuf::DescriptorPool::generated_pool()->FindServiceByName(service_name); ASSERT(service_desc != nullptr, fmt::format("{} missing", service_name)); @@ -200,7 +198,6 @@ TypeUrlToVersionedServiceMap* buildTypeUrlToServiceMap() { // services don't implement all, e.g. VHDS doesn't support SotW or REST. for (int method_index = 0; method_index < service_desc->method_count(); ++method_index) { const auto& method_desc = *service_desc->method(method_index); - const auto transport_api_version = versioned_service_name.first; if (absl::StartsWith(method_desc.name(), "Stream")) { service.sotw_grpc_.methods_[transport_api_version] = method_desc.full_name(); } else if (absl::StartsWith(method_desc.name(), "Delta")) { diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index f17d01decbc4f..2142b797b73ed 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -1,9 +1,8 @@ -#include "common/config/watch_map.h" - #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/cleanup.h" #include "common/config/decoded_resource_impl.h" +#include "common/config/watch_map.h" namespace Envoy { namespace Config { @@ -181,28 +180,27 @@ void WatchMap::onConfigUpdate( } // We just bundled up the updates into nice per-watch packages. Now, deliver them. - for (const auto& added : per_watch_added) { - const Watch* cur_watch = added.first; + for (const auto& [cur_watch, resource_to_add] : per_watch_added) { if (deferred_removed_during_update_->count(cur_watch) > 0) { continue; } const auto removed = per_watch_removed.find(cur_watch); if (removed == per_watch_removed.end()) { // additions only, no removals - cur_watch->callbacks_.onConfigUpdate(added.second, {}, system_version_info); + cur_watch->callbacks_.onConfigUpdate(resource_to_add, {}, system_version_info); } else { // both additions and removals - cur_watch->callbacks_.onConfigUpdate(added.second, removed->second, system_version_info); + cur_watch->callbacks_.onConfigUpdate(resource_to_add, removed->second, system_version_info); // Drop the removals now, so the final removals-only pass won't use them. per_watch_removed.erase(removed); } } // Any removals-only updates will not have been picked up in the per_watch_added loop. - for (auto& removed : per_watch_removed) { - if (deferred_removed_during_update_->count(removed.first) > 0) { + for (auto& [cur_watch, resource_to_remove] : per_watch_removed) { + if (deferred_removed_during_update_->count(cur_watch) > 0) { continue; } - removed.first->callbacks_.onConfigUpdate({}, removed.second, system_version_info); + cur_watch->callbacks_.onConfigUpdate({}, resource_to_remove, system_version_info); } } From 614de2fa0526325579315f1d1948ed5e26629c69 Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Tue, 4 Aug 2020 18:06:44 +0000 Subject: [PATCH 6/7] place the header in the right place Signed-off-by: Yifan Yang --- source/common/config/delta_subscription_state.cc | 3 ++- source/common/config/new_grpc_mux_impl.cc | 3 ++- source/common/config/type_to_endpoint.cc | 3 ++- source/common/config/watch_map.cc | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 0ca9bc3bfe8ec..c0a6a5502cb09 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -1,8 +1,9 @@ +#include "common/config/delta_subscription_state.h" + #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/assert.h" #include "common/common/hash.h" -#include "common/config/delta_subscription_state.h" #include "common/config/utility.h" namespace Envoy { diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index d9934ff57da51..b305cb1cd1fe1 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -1,9 +1,10 @@ +#include "common/config/new_grpc_mux_impl.h" + #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/assert.h" #include "common/common/backoff_strategy.h" #include "common/common/token_bucket_impl.h" -#include "common/config/new_grpc_mux_impl.h" #include "common/config/utility.h" #include "common/config/version_converter.h" #include "common/memory/utils.h" diff --git a/source/common/config/type_to_endpoint.cc b/source/common/config/type_to_endpoint.cc index 697b52b72721d..1c32fe47ad2c5 100644 --- a/source/common/config/type_to_endpoint.cc +++ b/source/common/config/type_to_endpoint.cc @@ -1,6 +1,7 @@ +#include "common/config/type_to_endpoint.h" + #include "envoy/annotations/resource.pb.h" -#include "common/config/type_to_endpoint.h" #include "common/grpc/common.h" // API_NO_BOOST_FILE diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index 2142b797b73ed..51e73e06344d9 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -1,8 +1,9 @@ +#include "common/config/watch_map.h" + #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/common/cleanup.h" #include "common/config/decoded_resource_impl.h" -#include "common/config/watch_map.h" namespace Envoy { namespace Config { From 295e0ac4a8a701461238940ea50c25ee1276b68b Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Fri, 7 Aug 2020 20:16:24 +0000 Subject: [PATCH 7/7] change a name Signed-off-by: Yifan Yang --- source/common/config/new_grpc_mux_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index b305cb1cd1fe1..131ccd24db51b 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -73,8 +73,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( } void NewGrpcMuxImpl::onStreamEstablished() { - for (auto& [type_url, sub] : subscriptions_) { - sub->sub_state_.markStreamFresh(); + for (auto& [type_url, subscription] : subscriptions_) { + subscription->sub_state_.markStreamFresh(); } trySendDiscoveryRequests(); } @@ -88,8 +88,8 @@ void NewGrpcMuxImpl::onEstablishmentFailure() { absl::flat_hash_map all_subscribed; absl::flat_hash_map already_called; do { - for (auto& [type_url, sub] : subscriptions_) { - all_subscribed[type_url] = &sub->sub_state_; + for (auto& [type_url, subscription] : subscriptions_) { + all_subscribed[type_url] = &subscription->sub_state_; } for (auto& sub : all_subscribed) { if (already_called.insert(sub).second) { // insert succeeded ==> not already called