Skip to content

Introduce string_view and use it for gpr_split_host_port#19488

Merged
soheilhy merged 4 commits intogrpc:masterfrom
soheilhy:string-view2
Jul 1, 2019
Merged

Introduce string_view and use it for gpr_split_host_port#19488
soheilhy merged 4 commits intogrpc:masterfrom
soheilhy:string-view2

Conversation

@soheilhy
Copy link
Copy Markdown
Contributor

@soheilhy soheilhy commented Jun 27, 2019

This is a roll forward of #19218 with a fix.

The issue was that instead of using the return value,
the original patch was checking the length of the host
and port.

The fix is as simple as the following two-line change.
Added unittests in test/core/gprpp/host_port_test.cc to
resolve the issue.

diff --git a/src/core/lib/gprpp/host_port.cc b/src/core/lib/gprpp/host_port.cc
index f3f8a73602..bb7ea5c391 100644
--- a/src/core/lib/gprpp/host_port.cc
+++ b/src/core/lib/gprpp/host_port.cc
@@ -90,8 +90,8 @@ bool SplitHostPort(StringView name, UniquePtr<char>* host,
   StringView host_view;
   StringView port_view;
   const bool ret = SplitHostPort(name, &host_view, &port_view);
-  host->reset(host_view.empty() ? nullptr : host_view.dup().release());
-  port->reset(port_view.empty() ? nullptr : port_view.dup().release());
+  host->reset(ret ? host_view.dup().release() : nullptr);
+  port->reset(ret ? port_view.dup().release() : nullptr);
   return ret;
 }
 }  // namespace grpc_core

@soheilhy soheilhy added the release notes: no Indicates if PR should not be in release notes label Jun 27, 2019
@soheilhy soheilhy requested a review from markdroth June 27, 2019 03:40
markdroth
markdroth reviewed Jun 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

When rolling forward a change with a fix, please use two separate commits: one for the revert of the previous commit, and a second one with the fixes. This makes it much easier for the reviewer to see what's changed since the last attempt. Thanks!

@soheilhy soheilhy force-pushed the string-view2 branch 2 times, most recently from 45b9dbf to 2f86ccc Compare June 27, 2019 15:28
@soheilhy
Copy link
Copy Markdown
Contributor Author

soheilhy commented Jun 27, 2019

Sorry about that @markdroth. I split the patch in two: first is the clean revert, and second is the fix. thanks for the review!

markdroth
markdroth approved these changes Jun 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

@soheilhy soheilhy mentioned this pull request Jun 27, 2019
Merged
@muxi muxi mentioned this pull request Jun 27, 2019
Merged
@soheilhy
Copy link
Copy Markdown
Contributor Author

soheilhy commented Jun 27, 2019

@markdroth sorry about the hassle, but I had to make two changes and rebase everything.

  1. I had to fix the credential_test.cc and revert in this PR.
  2. I had to fix the fix. Problem is that the code always returns a host string when it's able to split, but port will be nullptr if it's empty regardless of the return value.

I pushed the extra commits above, and will wait to see if there are other cases that I've missed. Sorry about the extra round-trip.

soheilhy added 4 commits June 27, 2019 20:56
@soheilhy
Revert "Revert "Introduce string_view and use it for gpr_split_host_p…
dbf88dd 
…ort.""

This reverts commit 80c177d.
@soheilhy
Fix SplitHostPort for empty strings.
c9ec1a6 
Add unittests to catch such errors in the future.
@soheilhy
Fix a backward compatibility bug.
fedf7e3 
To remain backward compatible, we only set port if it's a non-emptry
string.  But, we always store host.
@soheilhy
Revert "Fix build failure in credential_test.cc"
ea63c00 
This reverts commit dc858ee.
@markdroth
Copy link
Copy Markdown
Member

markdroth commented Jun 28, 2019

Still LGTM.

@soheilhy
Copy link
Copy Markdown
Contributor Author

soheilhy commented Jun 28, 2019

Thank you! Will request an internal cherrypick then.

@soheilhy
Copy link
Copy Markdown
Contributor Author

soheilhy commented Jul 1, 2019

Merging for cherry-pick.

@soheilhy soheilhy merged commit 39e982a into grpc:master Jul 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@markdroth markdroth markdroth approved these changes

@apolcyn apolcyn Awaiting requested review from apolcyn

@AspirinSJL AspirinSJL Awaiting requested review from AspirinSJL

Assignees

No one assigned

Labels

release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@soheilhy @markdroth