Does this mean that we were actually not using this error before?

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

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

  void UpdateState(grpc_connectivity_state state,
                   UniquePtr<SubchannelPicker> picker) override {
    if (parent_->resolver_ == nullptr) {

The bracket can be removed.

If the comment is kept, we still save one line.

It was never used for anything but human debugging. The main thing that changes in this PR is that we're no longer going to see the error message when the connectivity_state tracer is enabled. But since we will see the error message when the individual calls fail, I think that's okay.

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

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

It's generally considered a bad idea to have the body of an "if" statement be on its own line but not be surrounded by braces. This can lead to problems where a merge will incorrectly duplicate the body line with disasterous results, like the problem that Apple suffered in its SSL code a few years back:

https://embeddedgurus.com/barr-code/2014/03/apples-gotofail-ssl-security-bug-was-easily-preventable/

But I've changed it to fit the body on the same line with the condition, so this issue is moot.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

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

Yeah, in my hazy memory there is a story about merge like this. Thanks for the pointer!