Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lizan merged 6 commits into from
Apr 25, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ae9a25e
Replace base10 users of StringUtil::atoull with absl::SimpleAtoi
dnoe Apr 24, 2019
d0521e6
Add comment to StringUtil::atoull.
dnoe Apr 24, 2019
c1ef18d
Satisfy the pendantic spellchecker.
dnoe Apr 24, 2019
b02b73b
Add TODO for more string_viewification.
dnoe Apr 24, 2019
b803210
Merge branch 'master' into simple-atoi
dnoe Apr 25, 2019
da9a4ad
Fix asan.
dnoe Apr 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions source/common/access_log/access_log_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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));
}

Expand Down
3 changes: 3 additions & 0 deletions source/common/common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Member

@lizan lizan Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the remaining use cases of this method?

Copy link
Copy Markdown
Contributor Author

@dnoe dnoe Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are all hex conversions. I haven't dug around for the best option yet to replace them, but I plan to do that next as part of ongoing #6580 work.

I am also planning to eliminate StringUtil::atoll since every existing use case of it is base 10 and can just be replaced with absl::SimpleAtoi.

Expand Down
9 changes: 3 additions & 6 deletions source/common/grpc/common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnoe I think this can be removed now? Can you merge into one of your follow ups?

Copy link
Copy Markdown
Contributor Author

@dnoe dnoe Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

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);
}

Expand Down Expand Up @@ -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);
}
Expand Down
3 changes: 1 addition & 2 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions source/common/network/cidr_range.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions source/common/network/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions source/common/router/retry_state_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
}
}
Expand Down
8 changes: 2 additions & 6 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion source/common/runtime/runtime_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 1 addition & 3 deletions source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/common/fault/fault_config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 1 addition & 3 deletions source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
3 changes: 1 addition & 2 deletions source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand Down
7 changes: 3 additions & 4 deletions source/extensions/filters/http/gzip/gzip_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/filters/http/lua/lua_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand Down
8 changes: 4 additions & 4 deletions source/extensions/filters/network/redis_proxy/conn_pool_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(_, _))
Expand Down
6 changes: 2 additions & 4 deletions test/extensions/filters/http/gzip/gzip_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/filters/http/gzip/gzip_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions test/integration/autonomous_upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions tools/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ SIGFPE
SIGILL
SIGSEGV
SIGTERM
SimpleAtoi
SNI
SPDY
SPIFFE
Expand Down