Responding to code review comments

4 files changed, 33 insertions, 26 deletions

diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h
index 74c6672fd2..4fde0130c0 100644
--- a/src/core/lib/transport/metadata.h
+++ b/src/core/lib/transport/metadata.h
@@ -348,6 +348,4 @@ void grpc_mdctx_global_shutdown();
 /* {"www-authenticate", ""} */
 #define GRPC_MDELEM_WWW_AUTHENTICATE_EMPTY_INDEX 61
 
-/* Forward declarations */
-typedef struct grpc_mdelem grpc_mdelem;
 #endif /* GRPC_CORE_LIB_TRANSPORT_METADATA_H */
diff --git a/src/core/lib/transport/metadata_batch.cc b/src/core/lib/transport/metadata_batch.cc
index e2918fa08a..05f7a29e8a 100644
--- a/src/core/lib/transport/metadata_batch.cc
+++ b/src/core/lib/transport/metadata_batch.cc
@@ -114,7 +114,7 @@ static_hpack_table_metadata_info static_hpack_table_metadata[] = {
 };
 
 /* This is a faster check for seeing if a mdelem index is used or not. To verify
-   that the index value is valid, use 'is_valid_mdelem_index' */
+   that the index value is valid, use 'grpc_metadata_batch_is_valid_mdelem_index' */
 static bool is_mdelem_index_used(uint8_t index);
 
 static void set_mdelem_index_unused(uint8_t* index);
@@ -150,7 +150,7 @@ static void assert_valid_callouts(grpc_metadata_batch* batch) {
   for (grpc_linked_mdelem* l = batch->list.head; l != nullptr; l = l->next) {
     grpc_metadata_batch_callouts_index callout_idx;
     if (is_mdelem_index_used(l->md_index)) {
-      GPR_ASSERT(is_valid_mdelem_index(l->md_index));
+      GPR_ASSERT(grpc_metadata_batch_is_valid_mdelem_index(l->md_index));
       callout_idx = get_callouts_index(l);
       if (callout_idx != GRPC_BATCH_CALLOUTS_COUNT) {
         GPR_ASSERT(batch->idx.array[callout_idx] == l);
@@ -219,7 +219,6 @@ static grpc_error* maybe_link_callout(grpc_metadata_batch* batch,
     batch->idx.array[idx] = storage;
     return GRPC_ERROR_NONE;
   }
-
   grpc_error* err;
   if (is_mdelem_index_used(storage->md_index)) {
     char* message;
@@ -248,14 +247,14 @@ static void maybe_unlink_callout(grpc_metadata_batch* batch,
   batch->idx.array[idx] = nullptr;
 }
 
-bool is_valid_mdelem_index(uint8_t index) {
+bool grpc_metadata_batch_is_valid_mdelem_index(uint8_t index) {
   return index >= MIN_STATIC_HPACK_TABLE_IDX &&
          index <= MAX_STATIC_HPACK_TABLE_IDX;
 }
 
-bool is_mdelem_index_used(uint8_t index) { return index != 0; }
+static bool is_mdelem_index_used(uint8_t index) { return index != 0; }
 
-void set_mdelem_index_unused(uint8_t* index) {
+static void set_mdelem_index_unused(uint8_t* index) {
   GPR_ASSERT(index != nullptr);
   *index = 0;
 }
@@ -269,17 +268,17 @@ grpc_error* grpc_metadata_batch_add_head(grpc_metadata_batch* batch,
   return grpc_metadata_batch_link_head(batch, storage);
 }
 
-grpc_error* grpc_metadata_batch_add_head_index(grpc_metadata_batch* batch,
+grpc_error* grpc_metadata_batch_add_head_static(grpc_metadata_batch* batch,
                                                grpc_linked_mdelem* storage,
                                                uint8_t index_to_add) {
-  GPR_ASSERT(is_valid_mdelem_index(index_to_add));
+  GPR_ASSERT(grpc_metadata_batch_is_valid_mdelem_index(index_to_add));
   storage->md_index = index_to_add;
   return grpc_metadata_batch_link_head(batch, storage);
 }
 
 static void link_head(grpc_mdelem_list* list, grpc_linked_mdelem* storage) {
   assert_valid_list(list);
-  GPR_ASSERT(is_valid_mdelem_index(storage->md_index) ||
+  GPR_ASSERT(grpc_metadata_batch_is_valid_mdelem_index(storage->md_index) ||
              !GRPC_MDISNULL(storage->md));
   storage->prev = nullptr;
   storage->next = list->head;
@@ -315,17 +314,17 @@ grpc_error* grpc_metadata_batch_add_tail(grpc_metadata_batch* batch,
   return grpc_metadata_batch_link_tail(batch, storage);
 }
 
-grpc_error* grpc_metadata_batch_add_tail_index(grpc_metadata_batch* batch,
+grpc_error* grpc_metadata_batch_add_tail_static(grpc_metadata_batch* batch,
                                                grpc_linked_mdelem* storage,
                                                uint8_t index_to_add) {
-  GPR_ASSERT(is_valid_mdelem_index(index_to_add));
+  GPR_ASSERT(grpc_metadata_batch_is_valid_mdelem_index(index_to_add));
   storage->md_index = index_to_add;
   return grpc_metadata_batch_link_tail(batch, storage);
 }
 
 static void link_tail(grpc_mdelem_list* list, grpc_linked_mdelem* storage) {
   assert_valid_list(list);
-  GPR_ASSERT(is_valid_mdelem_index(storage->md_index) ||
+  GPR_ASSERT(grpc_metadata_batch_is_valid_mdelem_index(storage->md_index) ||
              !GRPC_MDISNULL(storage->md));
   storage->prev = list->tail;
   storage->next = nullptr;
@@ -486,7 +485,7 @@ void grpc_metadata_batch_copy(grpc_metadata_batch* src,
        elem = elem->next) {
     grpc_error* error = nullptr;
     if (is_mdelem_index_used(elem->md_index)) {
-      error = grpc_metadata_batch_add_tail_index(dst, &storage[i++],
+      error = grpc_metadata_batch_add_tail_static(dst, &storage[i++],
                                                  elem->md_index);
     } else {
       error = grpc_metadata_batch_add_tail(dst, &storage[i++],
diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h
index 25f9d0c14c..c0450684bb 100644
--- a/src/core/lib/transport/metadata_batch.h
+++ b/src/core/lib/transport/metadata_batch.h
@@ -110,12 +110,17 @@ grpc_error* grpc_metadata_batch_add_head(
     grpc_metadata_batch* batch, grpc_linked_mdelem* storage,
     grpc_mdelem elem_to_add) GRPC_MUST_USE_RESULT;
 
-/** Identical to grpc_metadata_batch_add_head, except takes the index of the
-    metadata element to add instead of a grpc_mdelem object. The index must
-    be a valid static hpack table index */
-grpc_error* grpc_metadata_batch_add_head_index(
+/** Add the static metadata element associated with \a elem_to_add_index
+    as the first element in \a batch, using \a storage as backing storage for 
+    the linked list element. \a storage is owned by the caller and must survive 
+    for the lifetime of batch. This usually means it should be around
+    for the lifetime of the call. 
+    Valid indices are in metadata.h (e.g., GRPC_MDELEM_STATUS_200_INDEX).
+    This is an optimization in the case where chttp2 is used as the underlying 
+    transport. */
+grpc_error* grpc_metadata_batch_add_head_static(
     grpc_metadata_batch* batch, grpc_linked_mdelem* storage,
-    uint8_t index_to_add) GRPC_MUST_USE_RESULT;
+    uint8_t elem_to_add_index) GRPC_MUST_USE_RESULT;
 
 /** Add \a elem_to_add as the last element in \a batch, using
     \a storage as backing storage for the linked list element.
@@ -127,10 +132,15 @@ grpc_error* grpc_metadata_batch_add_tail(
     grpc_metadata_batch* batch, grpc_linked_mdelem* storage,
     grpc_mdelem elem_to_add) GRPC_MUST_USE_RESULT;
 
-/** Identical to grpc_metadata_batch_add_tail, except takes the index of the
-    metadata element to add instead of a grpc_mdelem object. The index must
-    be a valid static hpack table index */
-grpc_error* grpc_metadata_batch_add_head_index(
+/** Add the static metadata element associated with \a elem_to_add_index
+    as the last element in \a batch, using \a storage as backing storage for 
+    the linked list element. \a storage is owned by the caller and must survive 
+    for the lifetime of batch. This usually means it should be around
+    for the lifetime of the call. 
+    Valid indices are in metadata.h (e.g., GRPC_MDELEM_STATUS_200_INDEX).
+    This is an optimization in the case where chttp2 is used as the underlying 
+    transport. */
+grpc_error* grpc_metadata_batch_add_tail_static(
     grpc_metadata_batch* batch, grpc_linked_mdelem* storage,
     uint8_t index_to_add) GRPC_MUST_USE_RESULT;
 
@@ -138,7 +148,7 @@ grpc_error* grpc_attach_md_to_error(grpc_error* src, grpc_mdelem md);
 
 /** Returns if the index is a valid static hpack table index, and thus if the
     grpc_linked_mdelem stores the index rather than the actual grpc_mdelem **/
-bool is_valid_mdelem_index(uint8_t index);
+bool grpc_metadata_batch_is_valid_mdelem_index(uint8_t index);
 
 /* Static hpack table metadata info */
 typedef struct static_hpack_table_metadata_info {
diff --git a/src/core/lib/transport/transport_op_string.cc b/src/core/lib/transport/transport_op_string.cc
index 7771b4d220..9f5dafa4af 100644
--- a/src/core/lib/transport/transport_op_string.cc
+++ b/src/core/lib/transport/transport_op_string.cc
@@ -48,7 +48,7 @@ static void put_metadata_list(gpr_strvec* b, grpc_metadata_batch md) {
   grpc_linked_mdelem* m;
   for (m = md.list.head; m != nullptr; m = m->next) {
     if (m != md.list.head) gpr_strvec_add(b, gpr_strdup(", "));
-    if (is_valid_mdelem_index(m->md_index)) {
+    if (grpc_metadata_batch_is_valid_mdelem_index(m->md_index)) {
       // TODO(hcaseyal): print out the mdelem index
       gpr_strvec_add(b, gpr_strdup("indexed mdelem"));
     } else {