Skip to content

Conversation

hennna
Copy link
Contributor

@hennna hennna commented Aug 7, 2018

Signed-off-by: Henna Huang henna@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Convert absl string_view to std::string in the dynamic metadata library. This is needed for the Google import.
Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Henna Huang <henna@google.com>
mattklein123
mattklein123 previously approved these changes Aug 7, 2018
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

sigh

@@ -6,7 +6,7 @@ namespace Envoy {
namespace RequestInfo {

void DynamicMetadataImpl::setData(absl::string_view data_name, std::unique_ptr<Object>&& data) {
if (data_storage_.find(data_name) != data_storage_.end()) {
if (data_storage_.find(std::string(data_name)) != data_storage_.end()) {
Copy link
Member

Choose a reason for hiding this comment

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

Please leave TODOs at each site to cleanup when we have full C++14 compliance.

Signed-off-by: Henna Huang <henna@google.com>
mattklein123
mattklein123 previously approved these changes Aug 7, 2018
htuch
htuch previously approved these changes Aug 7, 2018
@hennna
Copy link
Contributor Author

hennna commented Aug 7, 2018

@curiouserrandy

@@ -6,7 +6,8 @@ namespace Envoy {
namespace RequestInfo {

void DynamicMetadataImpl::setData(absl::string_view data_name, std::unique_ptr<Object>&& data) {
if (data_storage_.find(data_name) != data_storage_.end()) {
// TODO(Google): Remove string conversion when fixed internally.
if (data_storage_.find(std::string(data_name)) != data_storage_.end()) {
Copy link
Member

Choose a reason for hiding this comment

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

this function does same conversion 5 lines below, have a variable to store it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Henna Huang <henna@google.com>
@hennna hennna dismissed stale reviews from htuch and mattklein123 via 38fc87d August 8, 2018 00:55
@mattklein123 mattklein123 merged commit 699c008 into envoyproxy:master Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants