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");