From c7e92f26ebaecc8ea619de8b757034bb848b37d4 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 20 Nov 2018 15:03:12 -0800 Subject: Reviewer comments --- CMakeLists.txt | 2 ++ Makefile | 6 ++-- build.yaml | 3 ++ .../ext/transport/chttp2/transport/context_list.cc | 14 ++++----- .../ext/transport/chttp2/transport/context_list.h | 24 ++++++++-------- src/core/lib/iomgr/endpoint.cc | 5 +--- test/core/transport/chttp2/BUILD | 3 ++ test/core/transport/chttp2/context_list_test.cc | 33 +++++++++++++--------- tools/run_tests/generated/sources_and_headers.json | 2 ++ tools/run_tests/generated/tests.json | 2 +- 10 files changed, 52 insertions(+), 42 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1da78e8f57..7b36b3a5a8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12738,6 +12738,8 @@ target_link_libraries(context_list_test ${_gRPC_ALLTARGETS_LIBRARIES} grpc_test_util grpc + gpr_test_util + gpr ${_gRPC_GFLAGS_LIBRARIES} ) diff --git a/Makefile b/Makefile index ca867ebc46..d0f28281d7 100644 --- a/Makefile +++ b/Makefile @@ -17582,16 +17582,16 @@ $(BINDIR)/$(CONFIG)/context_list_test: protobuf_dep_error else -$(BINDIR)/$(CONFIG)/context_list_test: $(PROTOBUF_DEP) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a +$(BINDIR)/$(CONFIG)/context_list_test: $(PROTOBUF_DEP) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(E) "[LD] Linking $@" $(Q) mkdir -p `dirname $@` - $(Q) $(LDXX) $(LDFLAGS) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/context_list_test + $(Q) $(LDXX) $(LDFLAGS) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/context_list_test endif endif -$(OBJDIR)/$(CONFIG)/test/core/transport/chttp2/context_list_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a +$(OBJDIR)/$(CONFIG)/test/core/transport/chttp2/context_list_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a deps_context_list_test: $(CONTEXT_LIST_TEST_OBJS:.o=.dep) diff --git a/build.yaml b/build.yaml index cca5ed3cbf..772bdbbded 100644 --- a/build.yaml +++ b/build.yaml @@ -4605,6 +4605,7 @@ targets: - grpc++_codegen_base_src uses_polling: false - name: context_list_test + gtest: true build: test language: c++ src: @@ -4612,6 +4613,8 @@ targets: deps: - grpc_test_util - grpc + - gpr_test_util + - gpr uses_polling: false - name: credentials_test gtest: true diff --git a/src/core/ext/transport/chttp2/transport/context_list.cc b/src/core/ext/transport/chttp2/transport/context_list.cc index 11f5c14a39..4acd0c9583 100644 --- a/src/core/ext/transport/chttp2/transport/context_list.cc +++ b/src/core/ext/transport/chttp2/transport/context_list.cc @@ -21,30 +21,30 @@ #include "src/core/ext/transport/chttp2/transport/context_list.h" namespace { -void (*cb)(void*, grpc_core::Timestamps*) = nullptr; +void (*write_timestamps_callback_g)(void*, grpc_core::Timestamps*) = nullptr; } namespace grpc_core { void ContextList::Execute(void* arg, grpc_core::Timestamps* ts, grpc_error* error) { ContextList* head = static_cast(arg); - ContextList* ptr; + ContextList* to_be_freed; while (head != nullptr) { if (error == GRPC_ERROR_NONE && ts != nullptr) { - if (cb) { - cb(head->s_->context, ts); + if (write_timestamps_callback_g) { + write_timestamps_callback_g(head->s_->context, ts); } } GRPC_CHTTP2_STREAM_UNREF(static_cast(head->s_), "timestamp"); - ptr = head; + to_be_freed = head; head = head->next_; - grpc_core::Delete(ptr); + grpc_core::Delete(to_be_freed); } } void grpc_http2_set_write_timestamps_callback( void (*fn)(void*, grpc_core::Timestamps*)) { - cb = fn; + write_timestamps_callback_g = fn; } } /* namespace grpc_core */ diff --git a/src/core/ext/transport/chttp2/transport/context_list.h b/src/core/ext/transport/chttp2/transport/context_list.h index 0cf7ba4dc3..68d11e94d8 100644 --- a/src/core/ext/transport/chttp2/transport/context_list.h +++ b/src/core/ext/transport/chttp2/transport/context_list.h @@ -16,8 +16,8 @@ * */ -#ifndef GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H -#define GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H +#ifndef GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H +#define GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H #include @@ -33,8 +33,10 @@ class ContextList { * list. */ static void Append(ContextList** head, grpc_chttp2_stream* s) { /* Make sure context is not already present */ - ContextList* ptr = *head; GRPC_CHTTP2_STREAM_REF(s, "timestamp"); + +#ifndef NDEBUG + ContextList* ptr = *head; while (ptr != nullptr) { if (ptr->s_ == s) { GPR_ASSERT( @@ -43,17 +45,13 @@ class ContextList { } ptr = ptr->next_; } +#endif + + /* Create a new element in the list and add it at the front */ ContextList* elem = grpc_core::New(); elem->s_ = s; - if (*head == nullptr) { - *head = elem; - return; - } - ptr = *head; - while (ptr->next_ != nullptr) { - ptr = ptr->next_; - } - ptr->next_ = elem; + elem->next_ = *head; + *head = elem; } /* Executes a function \a fn with each context in the list and \a ts. It also @@ -69,4 +67,4 @@ void grpc_http2_set_write_timestamps_callback( void (*fn)(void*, grpc_core::Timestamps*)); } /* namespace grpc_core */ -#endif /* GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H */ +#endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H */ diff --git a/src/core/lib/iomgr/endpoint.cc b/src/core/lib/iomgr/endpoint.cc index 5e5effb2f1..06316c6031 100644 --- a/src/core/lib/iomgr/endpoint.cc +++ b/src/core/lib/iomgr/endpoint.cc @@ -63,8 +63,5 @@ grpc_resource_user* grpc_endpoint_get_resource_user(grpc_endpoint* ep) { } bool grpc_endpoint_can_track_err(grpc_endpoint* ep) { - if (ep->vtable->can_track_err != nullptr) { - return ep->vtable->can_track_err(ep); - } - return false; + return ep->vtable->can_track_err(ep); } diff --git a/test/core/transport/chttp2/BUILD b/test/core/transport/chttp2/BUILD index c7bfa1ec09..33437373e4 100644 --- a/test/core/transport/chttp2/BUILD +++ b/test/core/transport/chttp2/BUILD @@ -69,6 +69,9 @@ grpc_cc_test( grpc_cc_test( name = "context_list_test", srcs = ["context_list_test.cc"], + external_deps = [ + "gtest", + ], language = "C++", deps = [ "//:gpr", diff --git a/test/core/transport/chttp2/context_list_test.cc b/test/core/transport/chttp2/context_list_test.cc index 3814184e16..e2100899d3 100644 --- a/test/core/transport/chttp2/context_list_test.cc +++ b/test/core/transport/chttp2/context_list_test.cc @@ -18,6 +18,7 @@ #include "src/core/lib/iomgr/port.h" +#include #include #include @@ -29,25 +30,28 @@ #include -static void TestExecuteFlushesListVerifier(void* arg, - grpc_core::Timestamps* ts) { +namespace grpc_core { +namespace testing { +namespace { +void TestExecuteFlushesListVerifier(void* arg, grpc_core::Timestamps* ts) { GPR_ASSERT(arg != nullptr); gpr_atm* done = reinterpret_cast(arg); gpr_atm_rel_store(done, static_cast(1)); } -static void discard_write(grpc_slice slice) {} +void discard_write(grpc_slice slice) {} /** Tests that all ContextList elements in the list are flushed out on * execute. * Also tests that arg is passed correctly. */ -static void TestExecuteFlushesList() { +TEST(ContextList, ExecuteFlushesList) { grpc_core::ContextList* list = nullptr; grpc_http2_set_write_timestamps_callback(TestExecuteFlushesListVerifier); -#define NUM_ELEM 5 + const int kNumElems = 5; grpc_core::ExecCtx exec_ctx; grpc_stream_refcount ref; + GRPC_STREAM_REF_INIT(&ref, 1, nullptr, nullptr, "dummy ref"); grpc_resource_quota* resource_quota = grpc_resource_quota_create("context_list_test"); grpc_endpoint* mock_endpoint = @@ -55,9 +59,9 @@ static void TestExecuteFlushesList() { grpc_transport* t = grpc_create_chttp2_transport(nullptr, mock_endpoint, true); std::vector s; - s.reserve(NUM_ELEM); - gpr_atm verifier_called[NUM_ELEM]; - for (auto i = 0; i < NUM_ELEM; i++) { + s.reserve(kNumElems); + gpr_atm verifier_called[kNumElems]; + for (auto i = 0; i < kNumElems; i++) { s.push_back(static_cast( gpr_malloc(grpc_transport_stream_size(t)))); grpc_transport_init_stream(reinterpret_cast(t), @@ -69,7 +73,7 @@ static void TestExecuteFlushesList() { } grpc_core::Timestamps ts; grpc_core::ContextList::Execute(list, &ts, GRPC_ERROR_NONE); - for (auto i = 0; i < NUM_ELEM; i++) { + for (auto i = 0; i < kNumElems; i++) { GPR_ASSERT(gpr_atm_acq_load(&verifier_called[i]) == static_cast(1)); grpc_transport_destroy_stream(reinterpret_cast(t), @@ -82,12 +86,13 @@ static void TestExecuteFlushesList() { grpc_resource_quota_unref(resource_quota); exec_ctx.Flush(); } - -static void TestContextList() { TestExecuteFlushesList(); } +} // namespace +} // namespace testing +} // namespace grpc_core int main(int argc, char** argv) { + grpc_test_init(argc, argv); grpc_init(); - TestContextList(); - grpc_shutdown(); - return 0; + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); } diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 1581b9a94b..81af7829e5 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3509,6 +3509,8 @@ }, { "deps": [ + "gpr", + "gpr_test_util", "grpc", "grpc_test_util" ], diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 0569f81e6a..cc28e52ae2 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -4136,7 +4136,7 @@ "exclude_configs": [], "exclude_iomgrs": [], "flaky": false, - "gtest": false, + "gtest": true, "language": "c++", "name": "context_list_test", "platforms": [ -- cgit v1.2.3