From: Andres Freund <andres@anarazel.de>
To: Noah Misch <noah@leadboat.com>
Cc: 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
Date: Sun, 15 Sep 2019 21:47:52 -0700
Message-ID: <20190916044752.s5ajrua52tx5u2rt@alap3.anarazel.de> (raw)
In-Reply-To: <20190915221450.GA3500204@rfd.leadboat.com>
References: <E1i8y0J-00076R-Jj@gemulon.postgresql.org>
<20190915160021.GA24376@alvherre.pgsql>
<20190915221450.GA3500204@rfd.leadboat.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: andres@anarazel.de, noah@leadboat.com, alvherre@2ndquadrant.com, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit edge c
In-Reply-To: <20190916044752.s5ajrua52tx5u2rt@alap3.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox