This looks good overall! Comments are mostly minor.

I think @jiangtaoli2016 (or someone else from security) should also review the changes to the security code.

Please let me know if you have any questions.

Reviewed 33 of 33 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @soheilhy)

src/core/lib/gpr/host_port.h, line 46 at r1 (raw file):

                    which must later be destroyed using gpr_free().

   Prefer variant (1) over (2), because no allocation or copy is performed in

Since string_view has a dup() method, would it make sense to remove variant (2) and just require the caller to do the duplication when necessary?

src/core/lib/gpr/host_port.h, line 52 at r1 (raw file):

   Return true on success, false on failure. Guarantees *host and *port are
   cleared on failure. */
bool gpr_split_host_port(grpc_core::string_view name,

If this is going to use C++ APIs, maybe we should just convert this interface to idiomatic C++ and move it to the gprpp directory?

src/core/lib/gpr/string.cc, line 34 at r1 (raw file):

#include <grpc/support/log.h>
#include <grpc/support/string_util.h>
#include <limits>

I don't think we should use a C++ header in the gpr directory, since this is for C code only. Instead, I suggest using SIZE_MAX from stdint.h.

src/core/lib/gpr/string.cc, line 294 at r1 (raw file):

    ++b;
    --n;
  } while (ca == cb && ca && cb && n);

For readability, suggest && ca > 0 && cb > 0 && n > 0.

src/core/lib/gprpp/string_view.h, line 38 at r1 (raw file):

// Provides a light-weight view over a char array or a slice, similar to
// absl::string_view.

Please provide a comment similar to the one here:

https://github.com/grpc/grpc/blob/master/src/core/lib/gprpp/inlined_vector.h#L31

And then make sure the comment isn't lying. :)

If there are grpc-specific methods here that we would not expect in the absl implementation, maybe define this as StringView instead, so that when we convert to using absl, we can make StringView inherit from absl::string_view and add the new methods.

src/core/lib/gprpp/string_view.h, line 95 at r1 (raw file):

  // Return value is null-terminated and never nullptr.
  // Caller owns the return value and must free it using gpr_free.
  char* dup() const {

Maybe this should return UniquePtr<char>? (But see my comment above about being absl-compatible.)

src/core/lib/gprpp/string_view.h, line 102 at r1 (raw file):

  }

  int cmp(string_view other) const {

Make the arg a const reference?

src/core/lib/gprpp/string_view.h, line 111 at r1 (raw file):

};

inline bool operator==(string_view lhs, string_view rhs) {

Any reason not to make these member operator methods?

test/core/gprpp/string_view_test.cc, line 75 at r1 (raw file):

  grpc::testing::TestEnvironment env(argc, argv);
  ::testing::InitGoogleTest(&argc, argv);
  return RUN_ALL_TESTS();

Please make sure we have tests for every method of string_view. For such a basic API that will ultimately be used widely in our code-base, I think we need thorough test coverage.

Thank you for the great review @markdroth!

Reviewable status: 3 of 111 files reviewed, 9 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @markdroth)

src/core/lib/gpr/string.cc, line 34 at r1 (raw file):

Sure. Although I think are using even gprpp/ in gpr/ now, and IMHO we might want to allow such changes, since the code is already C++ (without stdlib runtime dependency).

src/core/lib/gpr/string.cc, line 294 at r1 (raw file):

Sure, changed them to != 0 for readability, which is a bit faster than > 0.

src/core/lib/gprpp/string_view.h, line 38 at r1 (raw file):

I updated the comment to reflect what is the intention. Yes, I agree the old comment was misleading.

src/core/lib/gprpp/string_view.h, line 95 at r1 (raw file):

Sure, that was in the first commit. Reverted to UniquePtr.

src/core/lib/gprpp/string_view.h, line 102 at r1 (raw file):

StringView is two pointers, and it's better passing it by value. Compiler is always able to optimize it out when necessary, and it is faster than const-ref. This is because const-ref is a pointer in itself, and any access would require a deference (and extra load). gpr_slice on the other hand is 4 pointers and that's why we should pass it by const-ref for performance.

I updated the doc to make this explicit. Thanks

src/core/lib/gprpp/string_view.h, line 111 at r1 (raw file):

That's a great question. This is because with free function we will accept natural comparisons such as "abc" == str_view which are not possible with member operators. I believe Effective C++ also recommends free function operators too.

test/core/gprpp/string_view_test.cc, line 75 at r1 (raw file):

Thank you for the pushback! It actually caught a bug. :-)

src/core/lib/gpr/host_port.h, line 46 at r1 (raw file):

That was the first commit I made, but it was really verbose due to the sheer usage of this method to interact with posix C API, expecting null-terminated string. So, I reverted that and kept this variant as a commonly-used helper. Please let me know if you have strong preference to avoid (2).

src/core/lib/gpr/host_port.h, line 52 at r1 (raw file):

That's a good idea. Moved to gprpp.

Thanks for doing this! Please let me know if you have any questions about any of this.

Reviewed 106 of 108 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @soheilhy)

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 88 at r3 (raw file):

#include "src/core/lib/channel/channel_stack.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Doesn't look like this is actually used here, so this can probably just be removed.

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 88 at r3 (raw file):

#include "src/core/lib/channel/channel_stack.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 42 at r3 (raw file):

#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_libuv.cc, line 30 at r3 (raw file):

#include "src/core/ext/filters/client_channel/server_address.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_windows.cc, line 30 at r3 (raw file):

#include "src/core/ext/filters/client_channel/server_address.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 35 at r3 (raw file):

#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc, line 37 at r3 (raw file):

#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gpr/useful.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc, line 34 at r3 (raw file):

#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 40 at r3 (raw file):

#include "src/core/lib/channel/handshaker.h"
#include "src/core/lib/channel/handshaker_registry.h"
#include "src/core/lib/gprpp/host_port.h"

Same here.

(I'll stop noting these individually, but please audit all instances of this include and remove the ones that aren't used.)

src/core/lib/gpr/string.cc, line 34 at r1 (raw file):

I see only two places in gpr where we're depending on something in gprpp.

One of them is the arena code, which was effectively moved to gprpp in #18786; the header in the gpr directory was retained only for backward compatibility, and @arjunroy said he'd clean up callers and remove the header from the gpr directory.

The second one is in log.cc, which uses the global config API to define the grpc_verbosity variable. This one seems wrong, and I think we should find a better way to deal with it.

Stepping back a bit, I think the original reason for the difference between gpr and gprpp was that gpr was C-only code and gprpp was C++ code. If we're going to keep maintaining them separately, then we should maintain that distinction, since it's the only real reason to keep them separate in the first place. Alternatively, we can decide that this distinction is no longer useful to us and combine both directories into one.

Anyway, we don't need to figure this out as part of this PR, but it is probably something we should discuss as a team.

src/core/lib/gprpp/string_view.h, line 38 at r1 (raw file):

I think the comment needs to make it clear that any API that is supported by absl::string_view must have the same semantics here as it does there. The only things we allow to be different from absl::string_view are the additional methods that we're adding to the absl API, and each of those methods should be clearly marked below.

src/core/lib/gprpp/string_view.h, line 115 at r3 (raw file):

    if (ret != 0) return ret;
    if (size() == other.size()) return 0;
    if (size() < other.size()) return -1;

For efficiency, wouldn't it make sense to check the sizes before the contents? That way, if they're not equal, we can fail faster.

test/core/gprpp/string_view_test.cc, line 143 at r3 (raw file):

TEST(StringViewTest, Find) {
  // Passing StringView::npos directly to GTEST macros result in link errors.

That's really strange. What causes the link error?

src/core/lib/gpr/host_port.h, line 46 at r1 (raw file):

I don't see that commit in the PR history, but having looked at a few callers, I see what you mean.

I'm fine with keeping variant (2) for readability reasons, but let's change its output parameters to UniquePtr<char>, so that the compiler can enforce ownership.

src/core/lib/gpr/host_port.h, line 52 at r1 (raw file):

Let's also convert it to idiomatic C++ (e.g., move it to the grpc_core namespace and change the name to something like SplitHostPort()).

Thank you for the review @markdorth. The change is invasive now and I couldn't test on window. We'll see if I've made all required windows changes. Otherwise, I'll push another commit tomorrow.

Reviewable status: 23 of 113 files reviewed, 14 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @markdroth)

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 88 at r3 (raw file):

Sure, but please note that this not related to this patch and is a pre-existing issue.

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 88 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 42 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_libuv.cc, line 30 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_windows.cc, line 30 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 35 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/fake/fake_resolver.cc, line 37 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc, line 34 at r3 (raw file):

Done.

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 40 at r3 (raw file):

Done.

src/core/lib/gpr/string.cc, line 34 at r1 (raw file):

Sure, let's discuss that in a gRPC meeting.

src/core/lib/gprpp/string_view.h, line 38 at r1 (raw file):

All good points. I updated the comment.

src/core/lib/gprpp/string_view.h, line 115 at r3 (raw file):

I'm probably misunderstanding your comment. This is cmp and not equals so we will have to compare the common prefix and then check the sizes. Can you please elaborate?

test/core/gprpp/string_view_test.cc, line 143 at r3 (raw file):

This is somewhere deep in GTEST macros and templates. AFAICT, this is because GTEST somehow ends up taking a ref of this symbol and for some reason clang doesn't insert the value. I didn't investigate further because I think it's a GTEST bug and we have a quick workaround here.

src/core/lib/gpr/host_port.h, line 46 at r1 (raw file):

Yes, sorry about that. I have forced-pushed for the first few rounds. I changed the UniquePtr<char> for char**. The change is really invasive as a result, although all mechanical. It also changes some internal uses, so I will have to cherry pick this individually I think.

src/core/lib/gpr/host_port.h, line 52 at r1 (raw file):

Sure, done.

Thanks for doing this! Just one remaining question.

Reviewed 87 of 90 files at r4, 4 of 4 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @soheilhy)

src/core/lib/gprpp/string_view.h, line 115 at r3 (raw file):

Can't we define cmp() such that if one operand is shorter than the other, it's considered smaller? Or is this meant for lexicographic sorting?

Thank you @markdroth. I'm still waiting for @jiangtaoli2016

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @markdroth)

src/core/lib/gprpp/string_view.h, line 115 at r3 (raw file):

My intention was to provide a replacement for strcmp() which provides lex sorting. The reason that we can't use strcmp directly is that the view may not null-terminated, hence we have this method.

Thanks Soheil very much for the changes. LGTM on security code.

Thank you for the review, Jiangtao!

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @jiangtaoli2016, and @markdroth)

src/core/lib/security/security_connector/ssl_utils.h, line 53 at r5 (raw file):

Thanks.

src/core/lib/security/security_connector/fake/fake_security_connector.cc, line 125 at r5 (raw file):

I changed this to target_name_override_ because it was null-terminated and also would contain the full string.
But, sure, changed it to fake_security_target_name_override_hostname.data().

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @jiangtaoli2016)