Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/client_channel.cc, line 899 at r1 (raw file):

p.second

I remember that it's suggested to explicitly create a local variable for the key and value fields in a pair before using them.

src/core/ext/filters/client_channel/client_channel.cc, line 1726 at r1 (raw file):

    }
    // Update health check service name used by existing subchannel wrappers.
    for (const auto& p : chand->subchannel_wrappers_) {

Why is auto valid here?

UpdateHealthCheckServiceName() is not const?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

src/core/ext/filters/client_channel/client_channel.cc, line 899 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/client_channel.cc, line 1726 at r1 (raw file):

I assume you're asking about const, not auto.

The const here means that the pointer will be const, but the object it's pointing to is not.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/client_channel.cc, line 1726 at r1 (raw file):

Yes, I wanted to say const.

src/core/ext/filters/client_channel/client_channel.cc, line 889 at r2 (raw file):

WatcherWrapper*&

Using WatcherWrapper* should work the same but simpler?

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

src/core/ext/filters/client_channel/client_channel.cc, line 889 at r2 (raw file):

We're resetting the value in the map on line 903, so it needs to be a reference.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)