Replace base10 users of StringUtil::atoull with absl::SimpleAtoi (#6697)

Description: `absl::SimpleAtoi` takes `absl::string_view` argument and can easily replace all of the calls to `StringUtil::atoull` that are using Base 10 conversion. This eliminates a significant number of places where a `std::string` needed to be constructed from `string_view` to obtain a C string for `StringUtil:atoull`. Risk Level: Low Testing: `bazel test //test/...` Docs Changes: N/A Release Notes: N/A Part of: Issue #6580 Signed-off-by: Dan Noé <dpn@google.com>
envoyproxy · Apr 25, 2019 · 71552f0 · 71552f0
1 parent ac32db3
commit 71552f0
Show file tree

Hide file tree

Showing 21 changed files with 41 additions and 55 deletions.
diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc
@@ -156,6 +156,7 @@ void AccessLogFormatParser::parseCommand(const std::string& token, const size_t
                                          const std::string& separator, std::string& main,
                                          std::vector<std::string>& sub_items,
                                          absl::optional<size_t>& max_length) {
+  // TODO(dnoe): Convert this to use string_view throughout.
   size_t end_request = token.find(')', start);
   sub_items.clear();
   if (end_request != token.length() - 1) {
@@ -169,10 +170,10 @@ void AccessLogFormatParser::parseCommand(const std::string& token, const size_t
       throw EnvoyException(fmt::format("Incorrect position of ')' in token: {}", token));
     }
 
-    std::string length_str = token.substr(end_request + 2);
+    const auto length_str = absl::string_view(token).substr(end_request + 2);
     uint64_t length_value;
 
-    if (!StringUtil::atoull(length_str.c_str(), length_value)) {
+    if (!absl::SimpleAtoi(length_str, &length_value)) {
       throw EnvoyException(fmt::format("Length must be an integer, given: {}", length_str));
     }
 

diff --git a/source/common/common/utility.h b/source/common/common/utility.h
@@ -154,6 +154,9 @@ class StringUtil {
 
   /**
    * Convert a string to an unsigned long, checking for error.
+   *
+   * Consider absl::SimpleAtoi instead if using base 10.
+   *
    * @param return true if successful, false otherwise.
    */
   static bool atoull(const char* str, uint64_t& out, int base = 10);

diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -57,10 +57,9 @@ void Common::chargeStat(const Upstream::ClusterInfo& cluster, const std::string&
                            grpc_status->value().getStringView()))
       .inc();
   uint64_t grpc_status_code;
-  const std::string grpc_status_string(grpc_status->value().getStringView());
   // TODO(dnoe): Migrate to pure string_view (#6580)
-  const bool success =
-      StringUtil::atoull(grpc_status_string.c_str(), grpc_status_code) && grpc_status_code == 0;
+  const bool success = absl::SimpleAtoi(grpc_status->value().getStringView(), &grpc_status_code) &&
+                       grpc_status_code == 0;
   chargeStat(cluster, protocol, grpc_service, grpc_method, success);
 }
 
@@ -88,9 +87,7 @@ absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap&
   if (!grpc_status_header || grpc_status_header->value().empty()) {
     return absl::optional<Status::GrpcStatus>();
   }
-  // TODO(dnoe): Migrate to pure string_view (#6580)
-  std::string grpc_status_header_string(grpc_status_header->value().getStringView());
-  if (!StringUtil::atoull(grpc_status_header_string.c_str(), grpc_status_code) ||
+  if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) ||
       grpc_status_code > Status::GrpcStatus::MaximumValid) {
     return absl::optional<Status::GrpcStatus>(Status::GrpcStatus::InvalidCode);
   }

diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc
@@ -238,8 +238,7 @@ bool Utility::hasSetCookie(const HeaderMap& headers, const std::string& key) {
 uint64_t Utility::getResponseStatus(const HeaderMap& headers) {
   const HeaderEntry* header = headers.Status();
   uint64_t response_code;
-  if (!header || !StringUtil::atoull(std::string(headers.Status()->value().getStringView()).c_str(),
-                                     response_code)) {
+  if (!header || !absl::SimpleAtoi(headers.Status()->value().getStringView(), &response_code)) {
     throw CodecClientException(":status must be specified and a valid unsigned long");
   }
   return response_code;

diff --git a/source/common/network/cidr_range.cc b/source/common/network/cidr_range.cc
@@ -128,8 +128,7 @@ CidrRange CidrRange::create(const std::string& range) {
     InstanceConstSharedPtr ptr = Utility::parseInternetAddress(std::string{parts[0]});
     if (ptr->type() == Type::Ip) {
       uint64_t length64;
-      const std::string part{parts[1]};
-      if (StringUtil::atoull(part.c_str(), length64, 10)) {
+      if (absl::SimpleAtoi(parts[1], &length64)) {
         if ((ptr->ip()->version() == IpVersion::v6 && length64 <= 128) ||
             (ptr->ip()->version() == IpVersion::v4 && length64 <= 32)) {
           return create(std::move(ptr), static_cast<uint32_t>(length64));

diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc
@@ -150,7 +150,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::
     const auto ip_str = ip_address.substr(1, pos - 1);
     const auto port_str = ip_address.substr(pos + 2);
     uint64_t port64 = 0;
-    if (port_str.empty() || !StringUtil::atoull(port_str.c_str(), port64, 10) || port64 > 65535) {
+    if (port_str.empty() || !absl::SimpleAtoi(port_str, &port64) || port64 > 65535) {
       throwWithMalformedIp(ip_address);
     }
     sockaddr_in6 sa6;
@@ -170,7 +170,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::
   const auto ip_str = ip_address.substr(0, pos);
   const auto port_str = ip_address.substr(pos + 1);
   uint64_t port64 = 0;
-  if (port_str.empty() || !StringUtil::atoull(port_str.c_str(), port64, 10) || port64 > 65535) {
+  if (port_str.empty() || !absl::SimpleAtoi(port_str, &port64) || port64 > 65535) {
     throwWithMalformedIp(ip_address);
   }
   sockaddr_in sa4;

diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc
@@ -85,10 +85,8 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap&
     retry_on_ |= parseRetryGrpcOn(request_headers.EnvoyRetryGrpcOn()->value().getStringView());
   }
   if (retry_on_ != 0 && request_headers.EnvoyMaxRetries()) {
-    // TODO(dnoe): Migrate to pure string_view (#6580)
-    const std::string max_retries(request_headers.EnvoyMaxRetries()->value().getStringView());
     uint64_t temp;
-    if (StringUtil::atoull(max_retries.c_str(), temp)) {
+    if (absl::SimpleAtoi(request_headers.EnvoyMaxRetries()->value().getStringView(), &temp)) {
       // The max retries header takes precedence if set.
       retries_remaining_ = temp;
     }
@@ -97,7 +95,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap&
     for (const auto code : StringUtil::splitToken(
              request_headers.EnvoyRetriableStatusCodes()->value().getStringView(), ",")) {
       uint64_t out;
-      if (StringUtil::atoull(std::string(code).c_str(), out)) {
+      if (absl::SimpleAtoi(code, &out)) {
         retriable_status_codes_.emplace_back(out);
       }
     }

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -148,9 +148,7 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he
   Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs();
   uint64_t header_timeout;
   if (header_timeout_entry) {
-    // TODO(dnoe): Migrate to pure string_view (#6580)
-    if (StringUtil::atoull(std::string(header_timeout_entry->value().getStringView()).c_str(),
-                           header_timeout)) {
+    if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) {
       timeout.global_timeout_ = std::chrono::milliseconds(header_timeout);
     }
     request_headers.removeEnvoyUpstreamRequestTimeoutMs();
@@ -159,9 +157,7 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he
   // See if there is a per try/retry timeout. If it's >= global we just ignore it.
   Http::HeaderEntry* per_try_timeout_entry = request_headers.EnvoyUpstreamRequestPerTryTimeoutMs();
   if (per_try_timeout_entry) {
-    // TODO(dnoe): Migrate to pure string_view (#6580)
-    if (StringUtil::atoull(std::string(per_try_timeout_entry->value().getStringView()).c_str(),
-                           header_timeout)) {
+    if (absl::SimpleAtoi(per_try_timeout_entry->value().getStringView(), &header_timeout)) {
       timeout.per_try_timeout_ = std::chrono::milliseconds(header_timeout);
     }
     request_headers.removeEnvoyUpstreamRequestPerTryTimeoutMs();

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -309,7 +309,7 @@ bool SnapshotImpl::parseEntryBooleanValue(Entry& entry) {
 
 bool SnapshotImpl::parseEntryUintValue(Entry& entry) {
   uint64_t converted_uint64;
-  if (StringUtil::atoull(entry.raw_string_value_.c_str(), converted_uint64)) {
+  if (absl::SimpleAtoi(entry.raw_string_value_, &converted_uint64)) {
     entry.uint_value_ = converted_uint64;
     return true;
   }

diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
@@ -216,9 +216,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::MessagePtr message) {
   // Set an error status if parsing status code fails. A Forbidden response is sent to the client
   // if the filter has not been configured with failure_mode_allow.
   uint64_t status_code{};
-  // TODO(dnoe): Migrate to pure string_view to eliminate std:string instance (#6580)
-  const std::string status_string(message->headers().Status()->value().getStringView());
-  if (!StringUtil::atoull(status_string.c_str(), status_code)) {
+  if (!absl::SimpleAtoi(message->headers().Status()->value().getStringView(), &status_code)) {
     ENVOY_LOG(warn, "ext_authz HTTP client failed to parse the HTTP status code.");
     return std::make_unique<Response>(errorResponse());
   }

diff --git a/source/extensions/filters/common/fault/fault_config.cc b/source/extensions/filters/common/fault/fault_config.cc
@@ -30,7 +30,7 @@ FaultDelayConfig::HeaderDelayProvider::duration(const Http::HeaderEntry* header)
   }
 
   uint64_t value;
-  if (!StringUtil::atoull(header->value().getStringView().data(), value)) {
+  if (!absl::SimpleAtoi(header->value().getStringView(), &value)) {
     return absl::nullopt;
   }
 
@@ -60,7 +60,7 @@ FaultRateLimitConfig::HeaderRateLimitProvider::rateKbps(const Http::HeaderEntry*
   }
 
   uint64_t value;
-  if (!StringUtil::atoull(header->value().getStringView().data(), value)) {
+  if (!absl::SimpleAtoi(header->value().getStringView(), &value)) {
     return absl::nullopt;
   }
 

diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc
@@ -71,9 +71,7 @@ Http::FilterTrailersStatus Http1BridgeFilter::encodeTrailers(Http::HeaderMap& tr
     const Http::HeaderEntry* grpc_status_header = trailers.GrpcStatus();
     if (grpc_status_header) {
       uint64_t grpc_status_code;
-      // TODO(dnoe): Migrate to pure string_view to eliminate std:string instance (#6580)
-      std::string grpc_status_code_string(grpc_status_header->value().getStringView());
-      if (!StringUtil::atoull(grpc_status_code_string.c_str(), grpc_status_code) ||
+      if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) ||
           grpc_status_code != 0) {
         response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable));
       }

diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc
@@ -33,8 +33,7 @@ void adjustContentLength(Http::HeaderMap& headers,
   auto length_header = headers.ContentLength();
   if (length_header != nullptr) {
     uint64_t length;
-    const std::string length_header_string(length_header->value().getStringView());
-    if (StringUtil::atoull(length_header_string.c_str(), length)) {
+    if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) {
       length_header->value(adjustment(length));
     }
   }

diff --git a/source/extensions/filters/http/gzip/gzip_filter.cc b/source/extensions/filters/http/gzip/gzip_filter.cc
@@ -232,10 +232,9 @@ bool GzipFilter::isMinimumContentLength(Http::HeaderMap& headers) const {
   const Http::HeaderEntry* content_length = headers.ContentLength();
   if (content_length) {
     uint64_t length;
-    // TODO(dnoe): Make StringUtil::atoull and friends string_view friendly.
-    const std::string content_length_str(content_length->value().getStringView());
-    const bool is_minimum_content_length = StringUtil::atoull(content_length_str.c_str(), length) &&
-                                           length >= config_->minimumLength();
+    const bool is_minimum_content_length =
+        absl::SimpleAtoi(content_length->value().getStringView(), &length) &&
+        length >= config_->minimumLength();
     if (!is_minimum_content_length) {
       config_->stats().content_length_too_small_.inc();
     }

diff --git a/source/extensions/filters/http/lua/lua_filter.cc b/source/extensions/filters/http/lua/lua_filter.cc
@@ -117,9 +117,9 @@ int StreamHandleWrapper::luaRespond(lua_State* state) {
   Http::HeaderMapPtr headers = buildHeadersFromTable(state, 2);
 
   uint64_t status;
-  const std::string status_string(headers->Status()->value().getStringView());
-  if (headers->Status() == nullptr || !StringUtil::atoull(status_string.c_str(), status) ||
-      status < 200 || status >= 600) {
+  if (headers->Status() == nullptr ||
+      !absl::SimpleAtoi(headers->Status()->value().getStringView(), &status) || status < 200 ||
+      status >= 600) {
     luaL_error(state, ":status must be between 200-599");
   }
 

diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc
@@ -170,9 +170,9 @@ InstanceImpl::ThreadLocalPool::makeRequestToHost(const std::string& host_address
   if (!ipv6) {
     host_address_map_key = host_address;
   } else {
-    const std::string ip_port = host_address.substr(colon_pos + 1);
+    const auto ip_port = absl::string_view(host_address).substr(colon_pos + 1);
     uint64_t ip_port_number;
-    if (!StringUtil::atoull(ip_port.c_str(), ip_port_number) || (ip_port_number > 65535)) {
+    if (!absl::SimpleAtoi(ip_port, &ip_port_number) || (ip_port_number > 65535)) {
       return nullptr;
     }
     try {
@@ -191,9 +191,9 @@ InstanceImpl::ThreadLocalPool::makeRequestToHost(const std::string& host_address
 
     if (!ipv6) {
       // Only create an IPv4 address instance if we need a new Upstream::HostImpl.
-      const std::string ip_port = host_address.substr(colon_pos + 1);
+      const auto ip_port = absl::string_view(host_address).substr(colon_pos + 1);
       uint64_t ip_port_number;
-      if (!StringUtil::atoull(ip_port.c_str(), ip_port_number) || (ip_port_number > 65535)) {
+      if (!absl::SimpleAtoi(ip_port, &ip_port_number) || (ip_port_number > 65535)) {
         return nullptr;
       }
       try {

diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc
@@ -84,7 +84,7 @@ class GrpcWebFilterTest : public testing::TestWithParam<std::tuple<std::string,
     EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _))
         .WillOnce(Invoke([=](Http::HeaderMap& headers, bool) {
           uint64_t code;
-          StringUtil::atoull(std::string(headers.Status()->value().getStringView()).c_str(), code);
+          ASSERT_TRUE(absl::SimpleAtoi(headers.Status()->value().getStringView(), &code));
           EXPECT_EQ(static_cast<uint64_t>(expected_code), code);
         }));
     EXPECT_CALL(decoder_callbacks_, encodeData(_, _))

diff --git a/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc b/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc
@@ -28,8 +28,7 @@ class GzipIntegrationTest : public testing::TestWithParam<Network::Address::IpVe
   void doRequestAndCompression(Http::TestHeaderMapImpl&& request_headers,
                                Http::TestHeaderMapImpl&& response_headers) {
     uint64_t content_length;
-    ASSERT_TRUE(
-        StringUtil::atoull(response_headers.get_("content-length").c_str(), content_length));
+    ASSERT_TRUE(absl::SimpleAtoi(response_headers.get_("content-length"), &content_length));
     const Buffer::OwnedImpl expected_response{std::string(content_length, 'a')};
     auto response =
         sendRequestAndWaitForResponse(request_headers, 0, response_headers, content_length);
@@ -54,8 +53,7 @@ class GzipIntegrationTest : public testing::TestWithParam<Network::Address::IpVe
   void doRequestAndNoCompression(Http::TestHeaderMapImpl&& request_headers,
                                  Http::TestHeaderMapImpl&& response_headers) {
     uint64_t content_length;
-    ASSERT_TRUE(
-        StringUtil::atoull(response_headers.get_("content-length").c_str(), content_length));
+    ASSERT_TRUE(absl::SimpleAtoi(response_headers.get_("content-length"), &content_length));
     auto response =
         sendRequestAndWaitForResponse(request_headers, 0, response_headers, content_length);
     EXPECT_TRUE(upstream_request_->complete());

diff --git a/test/extensions/filters/http/gzip/gzip_filter_test.cc b/test/extensions/filters/http/gzip/gzip_filter_test.cc
@@ -93,7 +93,7 @@ class GzipFilterTest : public testing::Test {
 
   void doResponseCompression(Http::TestHeaderMapImpl&& headers) {
     uint64_t content_length;
-    ASSERT_TRUE(StringUtil::atoull(headers.get_("content-length").c_str(), content_length));
+    ASSERT_TRUE(absl::SimpleAtoi(headers.get_("content-length"), &content_length));
     feedBuffer(content_length);
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(headers, false));
     EXPECT_EQ("", headers.get_("content-length"));
@@ -106,7 +106,7 @@ class GzipFilterTest : public testing::Test {
 
   void doResponseNoCompression(Http::TestHeaderMapImpl&& headers) {
     uint64_t content_length;
-    ASSERT_TRUE(StringUtil::atoull(headers.get_("content-length").c_str(), content_length));
+    ASSERT_TRUE(absl::SimpleAtoi(headers.get_("content-length"), &content_length));
     feedBuffer(content_length);
     Http::TestHeaderMapImpl continue_headers;
     EXPECT_EQ(Http::FilterHeadersStatus::Continue,

diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc
@@ -4,10 +4,10 @@ namespace Envoy {
 namespace {
 
 void HeaderToInt(const char header_name[], int32_t& return_int, Http::TestHeaderMapImpl& headers) {
-  std::string header_value = headers.get_(header_name);
+  const std::string header_value(headers.get_(header_name));
   if (!header_value.empty()) {
     uint64_t parsed_value;
-    RELEASE_ASSERT(StringUtil::atoull(header_value.c_str(), parsed_value, 10) &&
+    RELEASE_ASSERT(absl::SimpleAtoi(header_value, &parsed_value) &&
                        parsed_value < std::numeric_limits<int32_t>::max(),
                    "");
     return_int = parsed_value;

diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt
@@ -223,6 +223,7 @@ SIGFPE
 SIGILL
 SIGSEGV
 SIGTERM
+SimpleAtoi
 SNI
 SPDY
 SPIFFE