-
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
server: Move a few internal APIs from std::string to absl::string_view. #2889
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@ggreenway can you take a first pass at this? |
Signed-off-by: Joshua Marantz <jmarantz@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.
I think this looks good. One spot to potentially push deeper, but other than that LGTM.
source/server/http/admin.cc
Outdated
Buffer::Instance& response) { | ||
Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(url); | ||
Http::Utility::QueryParams query_params = Http::Utility::parseQueryString(std::string(url)); |
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.
You can avoid this conversion/allocation if you push one level deeper into this function. I think it would be straightforward without affecting other functions, at least at first glance. But up to you if you want to do that or not.
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.
Yeah I didn't need that construction. Good catch.
Signed-off-by: Joshua Marantz <jmarantz@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.
Thanks!
Signed-off-by: Joshua Marantz jmarantz@google.com
Description: Changes a few internal interfaces from std::string to absl::string_view, reducing the number of temporary allocations and memcpys that must be made.
Risk Level: Low
Testing: //test/..., tsan, asan, msan (sanitizers in progress)
Release Notes: N/A