From 1cfd81a604699d6ab8095c90e7da5fc588bb3048 Mon Sep 17 00:00:00 2001
From: Yash Tibrewal <yashkt@google.com>
Date: Tue, 21 Aug 2018 16:25:40 -0700
Subject: [PATCH] Explain the newer semantics of grpc_error_add_child

---
 src/core/lib/iomgr/error.cc   |  5 +++++
 src/core/lib/iomgr/error.h    |  7 +++++++
 test/core/iomgr/error_test.cc | 11 -----------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc
index dfd063a6959..13bc69ffb60 100644
--- a/src/core/lib/iomgr/error.cc
+++ b/src/core/lib/iomgr/error.cc
@@ -515,15 +515,20 @@ grpc_error* grpc_error_add_child(grpc_error* src, grpc_error* child) {
   GPR_TIMER_SCOPE("grpc_error_add_child", 0);
   if (src != GRPC_ERROR_NONE) {
     if (child == GRPC_ERROR_NONE) {
+      /* \a child is empty. Simply return the ref to \a src */
       return src;
     } else if (child != src) {
       grpc_error* new_err = copy_error_and_unref(src);
       internal_add_error(&new_err, child);
       return new_err;
     } else {
+      /* \a src and \a child are the same. Drop one of the references and return
+       * the other */
+      GRPC_ERROR_UNREF(child);
       return src;
     }
   } else {
+    /* \a src is empty. Simply return the ref to \a child */
     return child;
   }
 }
diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h
index e5369695c9f..49f4029bc22 100644
--- a/src/core/lib/iomgr/error.h
+++ b/src/core/lib/iomgr/error.h
@@ -185,6 +185,13 @@ bool grpc_error_get_str(grpc_error* error, grpc_error_strs which,
 /// error occurring. Allows root causing high level errors from lower level
 /// errors that contributed to them. The src error takes ownership of the
 /// child error.
+///
+/// Edge Conditions -
+/// 1) If either of \a src or \a child is GRPC_ERROR_NONE, returns a reference
+/// to the other argument. 2) If both \a src and \a child are GRPC_ERROR_NONE,
+/// returns GRPC_ERROR_NONE. 3) If \a src and \a child point to the same error,
+/// returns a single reference. (Note that, 2 references should have been
+/// received to the error in this case.)
 grpc_error* grpc_error_add_child(grpc_error* src,
                                  grpc_error* child) GRPC_MUST_USE_RESULT;
 
diff --git a/test/core/iomgr/error_test.cc b/test/core/iomgr/error_test.cc
index a1628a1f717..d78a8c2af39 100644
--- a/test/core/iomgr/error_test.cc
+++ b/test/core/iomgr/error_test.cc
@@ -187,16 +187,6 @@ static void test_os_error() {
   GRPC_ERROR_UNREF(error);
 }
 
-static void test_special() {
-  grpc_error* error = GRPC_ERROR_NONE;
-  error = grpc_error_add_child(
-      error, GRPC_ERROR_CREATE_FROM_STATIC_STRING("test child"));
-  intptr_t i;
-  GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &i));
-  GPR_ASSERT(i == GRPC_STATUS_OK);
-  GRPC_ERROR_UNREF(error);
-}
-
 static void test_overflow() {
   grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Overflow");
 
@@ -235,7 +225,6 @@ int main(int argc, char** argv) {
   test_os_error();
   test_create_referencing();
   test_create_referencing_many();
-  test_special();
   test_overflow();
   grpc_shutdown();