Responding to code review comments
diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc
index 8e9db0a..1880dbb 100644
--- a/src/core/ext/filters/http/server/http_server_filter.cc
+++ b/src/core/ext/filters/http/server/http_server_filter.cc
@@ -322,7 +322,7 @@
grpc_error* error = GRPC_ERROR_NONE;
static const char* error_name = "Failed sending initial metadata";
hs_add_error(error_name, &error,
- grpc_metadata_batch_add_head_index(
+ grpc_metadata_batch_add_head_static(
op->payload->send_initial_metadata.send_initial_metadata,
&calld->status, GRPC_MDELEM_STATUS_200_INDEX));
hs_add_error(error_name, &error,
diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
index f510a3f..3d6f150 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
@@ -1332,7 +1332,7 @@
bool is_client, bool is_initial) {
for (grpc_linked_mdelem* md = md_batch->list.head; md != nullptr;
md = md->next) {
- if (is_valid_mdelem_index(md->md_index)) {
+ if (grpc_metadata_batch_is_valid_mdelem_index(md->md_index)) {
gpr_log(GPR_INFO, "HTTP:%d:%s:%s: hpack table index: %d", id,
is_initial ? "HDR" : "TRL", is_client ? "CLI" : "SVR",
md->md_index);
diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
index c9ba74b..9e63144 100644
--- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
+++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
@@ -689,7 +689,7 @@
}
for (size_t i = 0; i < extra_headers_size; ++i) {
grpc_linked_mdelem* linked_md = extra_headers[i];
- if (is_valid_mdelem_index(linked_md->md_index)) {
+ if (grpc_metadata_batch_is_valid_mdelem_index(linked_md->md_index)) {
emit_indexed(c, linked_md->md_index, &st);
} else {
hpack_enc(c, linked_md->md, &st);
@@ -697,7 +697,7 @@
}
grpc_metadata_batch_assert_ok(metadata);
for (grpc_linked_mdelem* l = metadata->list.head; l; l = l->next) {
- if (is_valid_mdelem_index(l->md_index)) {
+ if (grpc_metadata_batch_is_valid_mdelem_index(l->md_index)) {
emit_indexed(c, l->md_index, &st);
} else {
hpack_enc(c, l->md, &st);
diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h
index 74c6672..4fde013 100644
--- a/src/core/lib/transport/metadata.h
+++ b/src/core/lib/transport/metadata.h
@@ -348,6 +348,4 @@
/* {"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 e2918fa..05f7a29 100644
--- a/src/core/lib/transport/metadata_batch.cc
+++ b/src/core/lib/transport/metadata_batch.cc
@@ -114,7 +114,7 @@
};
/* 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 25f9d0c..c045068 100644
--- a/src/core/lib/transport/metadata_batch.h
+++ b/src/core/lib/transport/metadata_batch.h
@@ -110,12 +110,17 @@
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_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 @@
/** 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 7771b4d..9f5dafa 100644
--- a/src/core/lib/transport/transport_op_string.cc
+++ b/src/core/lib/transport/transport_op_string.cc
@@ -48,7 +48,7 @@
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 {
