Обсуждение: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit edgec
Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases. Back-patch to 9.5, which introduced these functions. Reviewed by Tom Lane. Discussion: https://postgr.es/m/20190831071157.GA3251746@rfd.leadboat.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/f380c5190134a4e928bf30a4ff9fec775cdcc692 Modified Files -------------- src/test/regress/regress.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
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? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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");
Hi, On 2019-09-15 15:14:50 -0700, Noah Misch wrote: > 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) > + While it might be overkill on its own, it seems like it'd be a good utility to have for these kinds of tests. Unfortunately we can't easily make this type independent. The local variables are needed to avoid multiple evaluation. While we could infer their type using compiler specific magic (__typeof__() or C++), we'd still need to print them. We could however remove the local variables, and purely rely on stringification of the arguments for printing the error. > 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"); I'd name it EXPECT_EQ_U32 or such, but otherwise I think this is a clear improvement. Greetings, Andres Freund
On Sun, Sep 15, 2019 at 09:47:52PM -0700, Andres Freund wrote: > On 2019-09-15 15:14:50 -0700, Noah Misch wrote: > > --- 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) > > + > Unfortunately we can't easily make this type independent. The local > variables are needed to avoid multiple evaluation. While we could infer > their type using compiler specific magic (__typeof__() or C++), we'd > still need to print them. We could however remove the local variables, > and purely rely on stringification of the arguments for printing the > error. > I'd name it EXPECT_EQ_U32 or such, but otherwise I think this is a clear > improvement. EXPECT_EQ_U32 works for me; I mildly prefer that to a type-independent macro that doesn't print the unexpected value. Attached.