Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 1 Oct 2025 12:40:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Greg Burd <greg@burd.me>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aNyiu0fKRqihdwpV@paquier.xyz>
References: 
 <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
 <76DBEF5D-EA47-4C30-8D28-97B3B88E2E2D@greg.burd.me>
 <CAApHDvrvixMmscGEfEyYz0=qbwzM7Y0Up-Uh_AKVmfn8K2sn_A@mail.gmail.com>
 <aNxpxE6a9G3sbfrm@paquier.xyz>
 <CAApHDvoC-W3s2QJdBY6B32BXy2br9iGsNJ3v4uLFOosBgmY2YA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HXHPcvGWWoBgrN3A"
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvoC-W3s2QJdBY6B32BXy2br9iGsNJ3v4uLFOosBgmY2YA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aNyiu0fKRqihdwpV%40paquier.xyz>
Precedence: bulk


--HXHPcvGWWoBgrN3A
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Oct 01, 2025 at 01:07:10PM +1300, David Rowley wrote:
> On Wed, 1 Oct 2025 at 12:37, Michael Paquier <michael@paquier.xyz> wrote:
>> This is also something I've considered as embedding in a macro, and
>> discarded it.  The reason was code readability, because it gives to
>> the reader a quick knowledge of what can be freed once a call to a
>> specific bitmapset.c routine is done, acting as a kind of reference
>> template.  Here, the free call knowledge gets hidden.  test_bms_copy()
>> becomes more confusing to me.  This argument comes down to one's taste
>> and philosophy on the matter of test modules.  These are for me tools
>> of coverage, but also can work as reference templates when calling an
>> internal routine because we also document some of its implied
>> behaviors, like the inputs recycled.
>=20
> This sounds like another argument towards getting rid of the bms_frees
> from the module.

With the RETURN macro in mind, which itself cuts quite a little bit of
code, I'd rather do as you are suggesting and remove the free calls,
then.  Embedding this knowledge  in the return macro seems not much
adapted to me, but I don't object to this macro idea per-se.

> I'm not a huge fan of that part either. I didn't feel strongly enough
> to change it when adjusting Greg's v2. For me, I don't see the problem
> with leaving it up to nodeToString(), which will return "<>" for an
> empty set.

I think it's fine to leave it to nodeToString().  It is able to handle
a NULL input on its own.

> Thanks for explaining. Is anything being lost with the -1 return?
> Since that's not a valid member for a set, -1 can only be returned if
> bms_get_singleton_member() returned false.

+   if (!bms_get_singleton_member(bms, &member))
+       member =3D -1;

Hmm.  On second look, if you do that, there's no knowledge lost, I
guess.

> Because you're not happy with embedding the bms_free() in with
> PG_RETURN_BITMAPSET_AS_TEXT(), why don't we just get rid of all the
> bms_free's?  It's just a test module. If anyone ever comes complaining
> about memory usage then we can reconsider. I think it's nice to keep
> these wrappers as thin as possible in terms of the amount of C code.
> Having conditional logic in them, IMO, risk hiding the true behaviour
> of the underlying code that's being tested.

Okay.  Let's just remove as much code as possible, then.  This is
roughly what you have sent, minus the hardcoded empty set and the free
calls.  I'll just go do that in a bit.
--
Michael

--HXHPcvGWWoBgrN3A
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjcorsACgkQnvQgOdby
QH3A2g/8DJJkZeTQ2bBX7IGjnG7UdknDfjoupjOyT8ypxNtu1zCtw9uj8PZoQWFG
Ksn5yNUNAViSI6QTS2stDUBrf1/p70+3kGhiOEszK1bYt81oJOaYqyAdyDnQAV9r
LEvMA0h7RjfYglUcsaa1MDB8dptarQN+c1lHM3TDlOS9dj3Z3R4Ce1AwJdv3AyKn
6uy5v19T63jkQhQ7wZMkhZyxo9AoA8DI4QJDs9dEUOzJCH2Jcj8SyV4johyf1QX3
Vm8zHbq3ZUn3esKt4JaaekKIc+B2Vo/HVJV8QSOoEtRoGGCTspk0708PnVsQbyZL
dEqGbx6b3jQHm8kXwsiMFdYAATyGgl1KwmZggoy+LhMPlHjRwzWuE2YNUkf6mihA
1xqmFDSVLblf0RVFSw1e30ev2ZP2No9hzHuY4XNtMyqLYPrIfdgxkxKh23EvBjdb
ns8ezSDnckLOV5+ZI2a6tuwvKwEzq+kA9P79FnBFzOZ+2r7BD5p8wBTZWDuYKg5j
Ql4W/GXkOGT3EEOuzy8V8p9mzCFujcoqWGrM6iO67ws2JCQYvWOg6uqQnTbm6Mbe
HmNQhc4p8hyoQrsJFC9LNvMJ/b2fs95xciUgrN+XVCzzKmQktGfj6Z/y8+IMw+GR
DG4FRmFRBJNdJGQdIXXulOJbZ2G0EaWMeIdav7xVwVwSHJkwDAU=
=ugM4
-----END PGP SIGNATURE-----

--HXHPcvGWWoBgrN3A--