PR comments, round 3

5 files changed, 10 insertions, 19 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 6a10a25a32..bb29f65af9 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -1031,7 +1031,7 @@ static void create_subchannel_call_locked(grpc_call_element* elem,
 static void pick_done_locked(grpc_call_element* elem, grpc_error* error) {
   call_data* calld = (call_data*)elem->call_data;
   channel_data* chand = (channel_data*)elem->channel_data;
-  if (!calld->pick.connected_subchannel) {
+  if (calld->pick.connected_subchannel == nullptr) {
     // Failed to create subchannel.
     GRPC_ERROR_UNREF(calld->error);
     calld->error = error == GRPC_ERROR_NONE
@@ -1283,7 +1283,7 @@ static void start_pick_locked(void* arg, grpc_error* ignored) {
   grpc_call_element* elem = (grpc_call_element*)arg;
   call_data* calld = (call_data*)elem->call_data;
   channel_data* chand = (channel_data*)elem->channel_data;
-  GPR_ASSERT(!calld->pick.connected_subchannel);
+  GPR_ASSERT(calld->pick.connected_subchannel == nullptr);
   if (chand->lb_policy != nullptr) {
     // We already have an LB policy, so ask it for a pick.
     if (pick_callback_start_locked(elem)) {
@@ -1462,7 +1462,7 @@ static void cc_destroy_call_elem(grpc_call_element* elem,
                                "client_channel_destroy_call");
   }
   GPR_ASSERT(calld->waiting_for_pick_batches_count == 0);
-  if (calld->pick.connected_subchannel) {
+  if (calld->pick.connected_subchannel != nullptr) {
     calld->pick.connected_subchannel.reset();
   }
   for (size_t i = 0; i < GRPC_CONTEXT_COUNT; ++i) {
diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
index 39467272b3..9da21c7392 100644
--- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
@@ -322,7 +322,7 @@ static void pending_pick_set_metadata_and_context(pending_pick* pp) {
   /* if connected_subchannel is nullptr, no pick has been made by the RR
    * policy (e.g., all addresses failed to connect). There won't be any
    * user_data/token available */
-  if (pp->pick->connected_subchannel.get() != nullptr) {
+  if (pp->pick->connected_subchannel != nullptr) {
     if (!GRPC_MDISNULL(pp->lb_token)) {
       initial_metadata_add_lb_token(pp->pick->initial_metadata,
                                     &pp->pick->lb_token_mdelem_storage,
@@ -935,8 +935,7 @@ static void glb_shutdown_locked(grpc_lb_policy* pol,
       }
       gpr_free(pp);
     } else {
-      pp->pick->connected_subchannel =
-          grpc_core::MakeRefCounted<grpc_core::ConnectedSubchannel>(nullptr);
+      pp->pick->connected_subchannel.reset(nullptr);
       GRPC_CLOSURE_SCHED(&pp->on_complete, GRPC_ERROR_REF(error));
     }
     pp = next;
@@ -973,8 +972,7 @@ static void glb_cancel_pick_locked(grpc_lb_policy* pol,
   while (pp != nullptr) {
     pending_pick* next = pp->next;
     if (pp->pick == pick) {
-      pick->connected_subchannel =
-          grpc_core::MakeRefCounted<grpc_core::ConnectedSubchannel>(nullptr);
+      pick->connected_subchannel.reset(nullptr);
       GRPC_CLOSURE_SCHED(&pp->on_complete,
                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
                              "Pick Cancelled", &error, 1));
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index c91a305f33..725b78d478 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -294,7 +294,7 @@ static void pf_update_locked(grpc_lb_policy* policy,
                   p, p->selected->subchannel, i,
                   subchannel_list->num_subchannels);
         }
-        if (p->selected->connected_subchannel) {
+        if (p->selected->connected_subchannel != nullptr) {
           sd->connected_subchannel = p->selected->connected_subchannel;
         }
         p->selected = sd;
@@ -458,8 +458,6 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) {
       // Cases 1 and 2.
       grpc_connectivity_state_set(&p->state_tracker, GRPC_CHANNEL_READY,
                                   GRPC_ERROR_NONE, "connecting_ready");
-      sd->connected_subchannel =
-          grpc_subchannel_get_connected_subchannel(sd->subchannel);
       p->selected = sd;
       if (grpc_lb_pick_first_trace.enabled()) {
         gpr_log(GPR_INFO, "Pick First %p selected subchannel %p", (void*)p,
diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
index 91fee2d51a..dca345566a 100644
--- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
@@ -423,7 +423,7 @@ static void rr_connectivity_changed_locked(void* arg, grpc_error* error) {
       break;
     }
     case GRPC_CHANNEL_READY: {
-      if (!sd->connected_subchannel) {
+      if (sd->connected_subchannel == nullptr) {
         sd->connected_subchannel =
             grpc_subchannel_get_connected_subchannel(sd->subchannel);
       }
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index fe0bb486e8..5b8a14b9e7 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -56,10 +56,6 @@
 #define GRPC_SUBCHANNEL_RECONNECT_MAX_BACKOFF_SECONDS 120
 #define GRPC_SUBCHANNEL_RECONNECT_JITTER 0.2
 
-#define GET_CONNECTED_SUBCHANNEL(subchannel, barrier)          \
-  ((grpc_core::ConnectedSubchannel*)(gpr_atm_##barrier##_load( \
-      &(subchannel)->connected_subchannel)))
-
 namespace {
 struct state_watcher {
   grpc_closure closure;
@@ -443,7 +439,7 @@ static void maybe_start_connecting_locked(grpc_subchannel* c) {
     return;
   }
 
-  if (c->connected_subchannel) {
+  if (c->connected_subchannel != nullptr) {
     /* Already connected: don't restart */
     return;
   }
@@ -524,7 +520,7 @@ static void on_connected_subchannel_connectivity_changed(void* p,
   switch (connected_subchannel_watcher->connectivity_state) {
     case GRPC_CHANNEL_TRANSIENT_FAILURE:
     case GRPC_CHANNEL_SHUTDOWN: {
-      if (!c->disconnected && c->connected_subchannel) {
+      if (!c->disconnected && c->connected_subchannel != nullptr) {
         if (grpc_trace_stream_refcount.enabled()) {
           gpr_log(GPR_INFO,
                   "Connected subchannel %p of subchannel %p has gone into %s. "
@@ -605,7 +601,6 @@ static bool publish_transport_locked(grpc_subchannel* c) {
   /* publish */
   c->connected_subchannel.reset(
       grpc_core::New<grpc_core::ConnectedSubchannel>(stk));
-  gpr_atm_full_barrier();
   gpr_log(GPR_INFO, "New connected subchannel at %p for subchannel %p",
           c->connected_subchannel.get(), c);