-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Replace base10 users of StringUtil::atoull with absl::SimpleAtoi #6697
Conversation
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. Signed-off-by: Dan Noé <dpn@google.com>
Signed-off-by: Dan Noé <dpn@google.com>
Signed-off-by: Dan Noé <dpn@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just questions.
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
Signed-off-by: Dan Noé <dpn@google.com>
/retest |
🔨 rebuilding |
asan looks like a real issue, fixing. |
/retest |
🔨 rebuilding |
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Description:
absl::SimpleAtoi
takesabsl::string_view
argument and can easily replace all of the calls toStringUtil::atoull
that are using Base 10 conversion. This eliminates a significant number of places where astd::string
needed to be constructed fromstring_view
to obtain a C string forStringUtil: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