Date: Sun, 15 Sep 2019 15:14:50 -0700
From: Noah Misch <noah@leadboat.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit
 edge c
Message-ID: <20190915221450.GA3500204@rfd.leadboat.com>
References: <E1i8y0J-00076R-Jj@gemulon.postgresql.org>
 <20190915160021.GA24376@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190915160021.GA24376@alvherre.pgsql>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Sun, Sep 15, 2019 at 01:00:21PM -0300, Alvaro Herrera wrote:
> On 2019-Sep-14, Noah Misch wrote:
> > Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases.
> 
> I don't understand this.
> 
>     if (pg_atomic_fetch_add_u32(&var, INT_MAX) != INT_MAX)
>         elog(ERROR, "pg_atomic_add_fetch_u32() #3 wrong");
> 
>     pg_atomic_fetch_add_u32(&var, 2);   /* wrap to 0 */
> 
>     if (pg_atomic_fetch_add_u32(&var, PG_INT16_MAX) != 0)
>         elog(ERROR, "pg_atomic_fetch_add_u32() #3 wrong");
> 
> The existing one seems to be naming the wrong function in the error
> message, and if you fix that then you have two "#3 wrong" cases.
> Isn't this confusing?  Am I just too sensitive?  Is this pointless to
> fix?

It's a typo-class defect.  Would you like me to fix it, or would you like to
fix it?  I'd consider wrapping the tests in a macro (may be overkill, since
these tests don't change much):

--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -670,6 +670,16 @@ test_atomic_flag(void)
 	pg_atomic_clear_flag(&flag);
 }
 
+#define EXPECT(result_expr, expected_expr)		\
+	do { \
+		uint32		result = (result_expr); \
+		uint32		expected = (expected_expr); \
+		if (result != expected) \
+			elog(ERROR, \
+				 "%s yielded %u, expected %s in file \"%s\" line %u", \
+				 #result_expr, result, #expected_expr, __FILE__, __LINE__); \
+	} while (0)
+
 static void
 test_atomic_uint32(void)
 {
@@ -678,17 +688,10 @@ test_atomic_uint32(void)
 	int			i;
 
 	pg_atomic_init_u32(&var, 0);
-
-	if (pg_atomic_read_u32(&var) != 0)
-		elog(ERROR, "atomic_read_u32() #1 wrong");
-
+	EXPECT(pg_atomic_read_u32(&var), 0);
 	pg_atomic_write_u32(&var, 3);
-
-	if (pg_atomic_read_u32(&var) != 3)
-		elog(ERROR, "atomic_read_u32() #2 wrong");
-
-	if (pg_atomic_fetch_add_u32(&var, pg_atomic_read_u32(&var) - 2) != 3)
-		elog(ERROR, "atomic_fetch_add_u32() #1 wrong");
+	EXPECT(pg_atomic_read_u32(&var), 3);
+	EXPECT(pg_atomic_fetch_add_u32(&var, pg_atomic_read_u32(&var) - 2), 3);
 
 	if (pg_atomic_fetch_sub_u32(&var, 1) != 4)
 		elog(ERROR, "atomic_fetch_sub_u32() #1 wrong");