Re: [PATCH] Add tests for Bitmapset

On Wed, Oct 01, 2025 at 01:07:10PM +1300, David Rowley wrote:
> On Wed, 1 Oct 2025 at 12:37, Michael Paquier <michael(at)paquier(dot)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.
>
> 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 = -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